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.

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

Clear failures set in reset_failures #145

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

adamzapasnik
Copy link
Contributor

[8] pry(main)> Sidekiq::Failures.count
=> 4
[9] pry(main)> Sidekiq::Failures.reset_failures
=> [1, "OK"]
[10] pry(main)> Sidekiq::Failures.count
=> 0

Fixes #126

@mcasper
Copy link
Collaborator

mcasper commented Sep 8, 2022

Thank you for the contribution @adamzapasnik!

I think this might just be a case of ambiguous API naming, the current reset_failures method is only intended to clear out the failures count in the top level tabs (which it does today), as it's what the Reset Counter button uses. The failures themselves are still around, and that's what Sidekiq::Failures.count is returning (as you already know).

Perhaps a better solution here would be to rename reset_failures to reset_failures_counter?

@adamzapasnik
Copy link
Contributor Author

hey @mcasper thanks for the timely response

I was thinking about this particular thing, but I looked at the commit history 7331fea#diff-0b95b5843a26834e071c62e7ddfd651a5d090e870ecf2013774bb27a8caca9fbL56-L62 and previously (years ago :D) it was doing both things. Thus, I decided to reintroduce this behaviour.

Also re-reading https://github.com/mhfs/sidekiq-failures#reset-failures now, it seems to be outdated 🙈

Why did I open this PR in the first place?
I'd like to have a public API to reset the failures set. I know you can do it via web interface, but that's not optimal tbh. I can imagine such behaviour would be useful:

  1. if you wanted programmatically to reset the failures rest
  2. If you didn't have an access to web interface (for whatever reason, outage, no permissions, no web), you would be able to do it by invoking said method

@mcasper
Copy link
Collaborator

mcasper commented Sep 12, 2022

@adamzapasnik yeah fair point about the history, and those docs are definitely outdated 😆

I'd like to have a public API to reset the failures set.

There is actually a public API for clearing out the failures, though it looks like it's not documented currently, and it's in a different namespace 😬:

Sidekiq::Failures::FailureSet.new.clear

Given that, I would propose:

  1. Leave Sidekiq::Failures.reset_failures as is for backwards compatibility, mark it deprecated somehow
  2. Add a Sidekiq::Failures.reset_failure_count method to replace it
  3. Add a Sidekiq::Failures.clear_failures method that wraps Sidekiq::Failures::FailureSet.new.clear
  4. Document 1-3

Longer term I'm not convinced that the failures counter and the number of stored failures need to be independently controlled, but maybe there's some history there that I'm not aware of

@adamzapasnik
Copy link
Contributor Author

@mcasper sounds good, I'll try to wrap it up this week. (It's not urgent, but I'm thankful for your responsiveness.)

@adamzapasnik adamzapasnik force-pushed the fix-reset_failures branch 2 times, most recently from 8ceac3d to 3c1a67a Compare September 26, 2022 07:33
@adamzapasnik
Copy link
Contributor Author

@mcasper sorry for a bit long delay, I've forced pushed the changes, let me know what you think

@mcasper
Copy link
Collaborator

mcasper commented Oct 4, 2022

@adamzapasnik Looks great, thank you!

@mcasper mcasper merged commit 2b30cb1 into mhfs:master Oct 4, 2022
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.

Clearing Failures Not Working
2 participants