Stop using Modis index rpush:notifications:all to avoid storage leak#743
Draft
choznerol wants to merge 1 commit intorpush:masterfrom
Draft
Conversation
rpush#525 (comment) mentioned that `rpush:notifications:all` wouldn't be cleaned up properly. In our case, the `rpush:notifications:all` can keep growing to eat up most of our Redis storage (3.7GB out of 5GB in our case) If we're not using `Rpush::Client::Redis::Notification.all` anywhere, it might be a good idea to just avoid storing `rpush:notifications:all` at all. Ref for `enable_all_index`: - https://github.com/rpush/modis?tab=readme-ov-file#all-index - rpush/modis#7
rpush:notifications:all to avoid storage leakrpush:notifications:all to avoid storage leak
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Solution A for #744
Breaking change?
I didn't find any
Rpush::Client::Redis::Notification.allusage in rpush.and tests passed despite my change
I passed 563 tests locally with Rails 7.2 and Redis. (
CLIENT=redis bundle exec appraisal rails-7.2 rspec)but it would be more accurate if you could approve the CI run 🙏
If it's truly unused, it might be a good idea to just avoid maintaining
rpush:notifications:allat all.However, it's possible that RPush user is using
Rpush::Client::Redis::Notification.allfrom some reason. According to medis doc, they would start gettingIndexError. This might be considered a breaking change?Local testing result
For some reason, I COULDN'T really get rid of
rpush:notificaions:alllocallyI added some debug and sleep in test to inspect
all_index_enabled? == false,But Modis still work with
rpush:notifications:allfor some reason.I've double checked the doc and implementation but don't have an clue yet 🤔.
If this truly won't work, maybe Proposed Solution B or C from #744 is better.