Skip to content

Log the in progress work as a warning when force terminating.#1376

Merged
mperham merged 2 commits intosidekiq:masterfrom
jonhyman:feature/log-work-on-term
Dec 2, 2013
Merged

Log the in progress work as a warning when force terminating.#1376
mperham merged 2 commits intosidekiq:masterfrom
jonhyman:feature/log-work-on-term

Conversation

@jonhyman
Copy link
Contributor

@jonhyman jonhyman commented Dec 2, 2013

Helps trace through logs to understand what jobs got run more than once if a worker was forced to shutdown.

Helps trace through logs to understand what jobs got run more than once if a worker was forced to shutdown.
@mperham
Copy link
Collaborator

mperham commented Dec 2, 2013

Should it only log if necessary?

@jonhyman
Copy link
Contributor Author

jonhyman commented Dec 2, 2013

Do you mean moving the logging to before the t.raise Shutdown line? I thought if this log statement was executed, some job(s) was going to get run more than once.

@mperham
Copy link
Collaborator

mperham commented Dec 2, 2013

I mean log.warn {... } if necessary?

@jonhyman
Copy link
Contributor Author

jonhyman commented Dec 2, 2013

I think that any clause we put there will by and large always eval to true. E.g.,

logger.warn { ... } unless @busy.empty?

should still log since we're in the hard_shutdown_in method, meaning that busy is non-empty (otherwise clean_up_for_graceful_shutdown would have called shutdown)

I think it's possible that @busy finishes while this code is being called, so I can update to guard against that but I think it'll almost always eval to true.

Is there some other guard condition I'm not thinking of?

@mperham
Copy link
Collaborator

mperham commented Dec 2, 2013

busy can't change while we're executing. Actors are exclusive - the manager manages its own internal state and it only processes one message at a time.

I see what you are saying though. If busy is non-empty, in_progress should be non-empty too. Log in_progress on a separate log statement and I think that's fine.

@jonhyman
Copy link
Contributor Author

jonhyman commented Dec 2, 2013

updated

mperham added a commit that referenced this pull request Dec 2, 2013
Log the in progress work as a warning when force terminating.
@mperham mperham merged commit 4c15aca into sidekiq:master Dec 2, 2013
@jonhyman jonhyman deleted the feature/log-work-on-term branch December 2, 2013 22:50
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