Skip to content

Add rake task to delete stats keys set to 0#250

Merged
bors[bot] merged 8 commits intomasterfrom
rake-task-to-delete-stats-with-0
Jan 27, 2021
Merged

Add rake task to delete stats keys set to 0#250
bors[bot] merged 8 commits intomasterfrom
rake-task-to-delete-stats-with-0

Conversation

@davidor
Copy link
Copy Markdown
Contributor

@davidor davidor commented Jan 26, 2021

This PR adds a rake task to delete the stats keys set to 0. Those keys are useless and occupy Redis memory unnecessarily. They were generated by the bug that was fixed in #247

@davidor davidor requested a review from unleashed January 26, 2021 15:45
Copy link
Copy Markdown
Contributor

@unleashed unleashed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out the comments for some minor changes needed - also s/func/method/g in commit messages :)

Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
@davidor davidor force-pushed the rake-task-to-delete-stats-with-0 branch from 10dd24c to 7e5d339 Compare January 26, 2021 18:06
Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
@davidor davidor force-pushed the rake-task-to-delete-stats-with-0 branch from 5d8a9e1 to 849ac3f Compare January 27, 2021 13:43
@davidor
Copy link
Copy Markdown
Contributor Author

davidor commented Jan 27, 2021

@unleashed please check the 3 new commits.

@davidor davidor requested a review from unleashed January 27, 2021 13:53
Copy link
Copy Markdown
Contributor

@unleashed unleashed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but check the few comments I made. I think there is some more room for improvement but the affected code does not merit the extra effort.

Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
retries ||= 0
return yield
rescue Exception => e
retry if (retries += 1) < MAX_RETRIES_REDIS_ERRORS
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😲 This line is a bit surprising. How is this following your usual readability policy? Why don't you just extract the increment to a statement before the retry check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion about this. You would have complained either way :trollface:

Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
Comment thread lib/3scale/backend/stats/cleaner.rb Outdated
@davidor davidor force-pushed the rake-task-to-delete-stats-with-0 branch from c836a3d to 25d3766 Compare January 27, 2021 15:49
@davidor
Copy link
Copy Markdown
Contributor Author

davidor commented Jan 27, 2021

bors r=@unleashed

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Jan 27, 2021

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@davidor
Copy link
Copy Markdown
Contributor Author

davidor commented Jan 27, 2021

bors retry

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Jan 27, 2021

Build succeeded:

@bors bors Bot merged commit bf93a4f into master Jan 27, 2021
@bors bors Bot deleted the rake-task-to-delete-stats-with-0 branch January 27, 2021 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants