-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: close connections on exit (without another map) #1262
Conversation
045203e
to
55cd47e
Compare
Looks great :) It does look like nsqlookupd could use similar treatment. |
55cd47e
to
02e4247
Compare
@ploxiln I don't remember what I was trying to do here with the latest commit 😬, so I'll ping when it's ready for review. |
d6a79f6
to
58a5dac
Compare
alright I think this is ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Seems like going through the protocol.Client
interface causes a couple more type-assertions than would otherwise be needed, and might be avoidable ... but really not a big deal, it's fine :)
(tested a bit locally too, seems to work) |
78c5ac1
to
cc3d7e1
Compare
Wow, that GitHub feature to "commit suggestion" is 😍 PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 squash?
2e86e20
to
11b3d89
Compare
squashed |
Going through the backlog of PRs that have landed, I propose a simpler alternative to #1198, which fixes a bug introduced in #1190.
We're already tracking all client connections, including producers in
nsqd.clients
, so let's just use it to trigger connection close on exit.Note: we should probably do this consistently across
nsqd
andnsqlookupd
w/r/t what struct owns tracking and closing client conns, so maybe I should make a similar set of changes tonsqlookupd
.cc @ploxiln