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

nsqd: bump go-svc to properly handle SIGTERM #757

Merged
merged 1 commit into from
May 5, 2016

Conversation

judwhite
Copy link
Contributor

@judwhite judwhite commented May 5, 2016

@mreiferson mreiferson changed the title Handle SIGTERM nsqd: bump go-svc to properly handle SIGTERM May 5, 2016
@mreiferson
Copy link
Member

thanks for the quick fix @judwhite

@mreiferson mreiferson added the bug label May 5, 2016
@mreiferson mreiferson merged commit 45126e1 into nsqio:master May 5, 2016
@mreiferson
Copy link
Member

will issue a release, since this is a critical fix.

@mreiferson
Copy link
Member

cc @jehiah

@jehiah
Copy link
Member

jehiah commented May 5, 2016

👍

@ploxiln
Copy link
Member

ploxiln commented May 7, 2016

I did some experimentation (in preparation of rolling out a new build which is now running fine in production) and found that this issue is not as serious as it sounds (to someone less familiar with nsqd internals, like me).

nsqd 0.3.7 saves its metadata whenever a new channel is created, so even if it fails to save on exit, the topics/channels metadata is pretty much always up-to-date anyway, and new channels persist after an ungraceful restart.

@mreiferson
Copy link
Member

@ploxiln the problem is that it also won't persist messages in memory to disk

@ploxiln
Copy link
Member

ploxiln commented May 7, 2016

yeah, that's bad, ignore me 👅

@mreiferson
Copy link
Member

for anyone following along at home, please read:

https://groups.google.com/forum/#!topic/nsq-users/yOckPLUWi4Q

@mreiferson
Copy link
Member

@judwhite thinking about this more, particularly the genesis of the regression, it comes down to the fact that this signal handling code shouldn't be part of go-svc.

Applications depending on go-svc should pass in a channel that can trigger "exit". I even pointed this out on the original PR, but I neglected to confirm that the code moved over without modification 😦 .

@judwhite
Copy link
Contributor Author

judwhite commented May 8, 2016

My mistake.

@mreiferson
Copy link
Member

It's all good, shit happens, I missed it too. Would it be possible to make those changes?

@judwhite
Copy link
Contributor Author

judwhite commented May 8, 2016

Yeah, I'll open a PR at go-svc and a corresponding one here to pass the signals. I'll tag you when it's ready.

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

Successfully merging this pull request may close these issues.

4 participants