Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add rake task for cleaning up sidekiq batch keys #3912

Merged
merged 3 commits into from
Oct 14, 2024
Merged

Add rake task for cleaning up sidekiq batch keys #3912

merged 3 commits into from
Oct 14, 2024

Conversation

mayorova
Copy link
Contributor

@mayorova mayorova commented Oct 9, 2024

What this PR does / why we need it:

Add a rake task to cleanup BID-* keys.

Which issue(s) this PR fixes

Related to https://issues.redhat.com/browse/THREESCALE-8124

Verification steps

bundle exec rake sidekiq:cleanup_batches

Special notes for your reviewer:

akostadinov
akostadinov reviewed Oct 9, 2024
View reviewed changes
app/services/sidekiq_batch_cleanup_service.rb Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather put this under lib/ because it is not used by the server at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, true!

@mayorova
Copy link
Contributor Author

mayorova commented Oct 9, 2024

@akostadinov I applied some changes...
With regards to the "Pattern", I am having some issues with requiring that dependency 🙃 ThreeScale::Patterns::Service specifically

@akostadinov
Copy link
Contributor

Ah, yeah, I added as ThreeScale::Patterns::Service. What is the issue? I think it is a good idea to have a precise "service" definition. Even if it is different than what we have in ThreeScale::Patterns::Service. Maybe update ThreeScale::Patterns::Service the way it makes more sense to you?

akostadinov
akostadinov reviewed Oct 10, 2024
View reviewed changes
curl-output.txt Outdated Show resolved Hide resolved
@mayorova mayorova force-pushed the cleanup-sidekiq-batch-keys branch from c261046 to a96314b Compare October 10, 2024 10:47
@mayorova mayorova changed the title Add rake task for cleaning up sidekiq batch keys! Add rake task for cleaning up sidekiq batch keys Oct 10, 2024
@mayorova mayorova force-pushed the cleanup-sidekiq-batch-keys branch from 81923e2 to af428f2 Compare October 11, 2024 09:25
@mayorova @akostadinov
Add rake task for cleaning up sidekiq batch keys
52504ea 
Apply suggestions from code review

Co-authored-by: Aleksandar N. Kostadinov <akostadinov@gmail.com>
@mayorova mayorova force-pushed the cleanup-sidekiq-batch-keys branch from af428f2 to 52504ea Compare October 11, 2024 09:34
@mayorova mayorova marked this pull request as ready for review October 11, 2024 09:34
@mayorova mayorova requested a review from akostadinov October 11, 2024 09:44
akostadinov
akostadinov reviewed Oct 11, 2024
View reviewed changes
lib/tasks/sidekiq/cleanup.rake Outdated
params = args[:max_age_seconds] ? { max_age_seconds: Integer(args[:max_age_seconds]) } : {}

message = params[:max_age_seconds] ? "#{params[:max_age_seconds].seconds.in_hours} hours" : "the default age"
puts "Cleaning up the sidekiq-batch BID-* keys older than #{message}"
Copy link
Contributor

@akostadinov akostadinov Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think that we don't need this logging given that SidekiqBatchCleanupService already logs it with the actual value, not just the default age". btw putsfor logging is not recommended. Using a logger or at leastwarn` is better becaue it will not go to stdout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

akostadinov
akostadinov approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@akostadinov akostadinov left a comment

Choose a reason for hiding this comment

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

Thanks!

@mayorova mayorova merged commit 8f44394 into master Oct 14, 2024
17 of 21 checks passed
@mayorova mayorova deleted the cleanup-sidekiq-batch-keys branch October 14, 2024 07:55
josemigallas pushed a commit that referenced this pull request Oct 14, 2024
@mayorova @akostadinov @josemigallas
Add rake task for cleaning up sidekiq batch keys (#3912)
a9cd3e3 
* Add rake task for cleaning up sidekiq batch keys

Apply suggestions from code review

Co-authored-by: Aleksandar N. Kostadinov <akostadinov@gmail.com>

* Remove unnecessary logs

* Refactoring

---------

Co-authored-by: Aleksandar N. Kostadinov <akostadinov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akostadinov akostadinov akostadinov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mayorova @akostadinov