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

SIGKILL handling feature #486

Closed
mhoncharov opened this issue Apr 3, 2020 · 6 comments
Closed

SIGKILL handling feature #486

mhoncharov opened this issue Apr 3, 2020 · 6 comments

Comments

@mhoncharov
Copy link

Hi,

After a pretty long investigation, I've found interesting thing about handling SIGKILL or any other accidents inside SIdekiq and SidekiqUniqueJobs.

For now, if Sidekiq server was killed and some jobs were lost during this process (not returned to the queue) there are persisted unique digests which are not cleanable (except the case when it has TTL). It becomes a huge problem on Heroku servers because they are restarting their instances with SIGKILL after 30 seconds of SIGTERM running.

I've made a pretty simple solution which should handle this situation.
It's here
https://github.com/restaurant-cheetah/sidekiq_unique_digests_cleaner

For now, it supports only version ~>6, but I'm going to extend it for older ones (if needed) and for 7.x).

Are you interested in such a feature inside the gem? Or, probably, it's better to handle it outside? Not sure how common the described issue is.

Related ones:

#362
#14

@mhenrixon
Copy link
Owner

Hi,

One of the reasons for v7 was to provide reaping of keys that should be removed but for whatever reason wasn’t so I’m not sure it makes sense for v7.

That said I’m going to have a look for v6 and possibly v5 (though not as many problems there).

I’ll get back to you later

@mhenrixon
Copy link
Owner

mhenrixon commented Apr 4, 2020

So I had a look at the code, I like how simple it is @mxhoncharov :) I'm not sure how performant it is for really large systems but I'm happy to support something like this for v6. For v7 I think I'll keep improving on the existing solution because it is already a little more hmm what is the word I'm looking for... sophisticated!

@mhoncharov
Copy link
Author

@mhenrixon nice to hear :)
For now, we are a bit limited for using 7.x version, but it will be great if there will be such a handling system.

About performance, I think it's the best option to call it once during Sidekiq initialization. I haven't researched all the unique jobs architecture yet. So, I can't to make anything more elegant :) And not sure, that there is such need.

I'm really not sure how critical is this problem for usual using unique jobs. Possibly, it will be good enough to add a link for this repo into README?
I really believe that your implementation will be more sophisticated and it should be because it it's a part of a system.

I will add support for older versions. The main difference there, if I remember correctly, only inside logger's API for Sidekiq and correct method for deleting digests. I'm not even sure that it's different for versions older than 6.x, I've seen that 7.x has a new architecture. So, I will check it and possibly, it might work correctly on older versions.

@simonoff
Copy link
Contributor

@mhenrixon unfortunately reaper not cleaning the locks... even it possible to found a very old or them not cleared when on UI nothing, but when trying to fetch from from REPL - they are available.

@mhenrixon
Copy link
Owner

@simonoff if the job is retried, scheduled, or being processed the keys will not be reaped (the job is considered active). Could that be the case? There are ways to debug what is going on if you care to help me out getting it solid.

@mhenrixon
Copy link
Owner

#571 v7.0.1 is highly recommended, I believe I fixed all the quirks about the reaping. Let me know if you find any type of issues with it and I will fix it immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants