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: close connections on exit (without another map) #1262

Merged
merged 4 commits into from
Mar 1, 2021

Conversation

mreiferson
Copy link
Member

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 and nsqlookupd w/r/t what struct owns tracking and closing client conns, so maybe I should make a similar set of changes to nsqlookupd.

cc @ploxiln

@mreiferson mreiferson added the bug label Jun 14, 2020
@mreiferson mreiferson requested a review from ploxiln June 14, 2020 04:14
@ploxiln
Copy link
Member

ploxiln commented Jun 15, 2020

Looks great :)

It does look like nsqlookupd could use similar treatment.

nsqd/client_v2.go Outdated Show resolved Hide resolved
nsqd/nsqd.go Outdated Show resolved Hide resolved
@mreiferson
Copy link
Member Author

@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.

@mreiferson mreiferson self-assigned this Nov 25, 2020
@mreiferson mreiferson force-pushed the nsqd-client-close branch 2 times, most recently from d6a79f6 to 58a5dac Compare December 30, 2020 06:38
@mreiferson
Copy link
Member Author

alright I think this is ready

Copy link
Member

@ploxiln ploxiln left a 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 :)

@ploxiln
Copy link
Member

ploxiln commented Dec 30, 2020

(tested a bit locally too, seems to work)

@mreiferson mreiferson force-pushed the nsqd-client-close branch 2 times, most recently from 78c5ac1 to cc3d7e1 Compare January 1, 2021 01:27
nsqd/client_v2.go Outdated Show resolved Hide resolved
@mreiferson
Copy link
Member Author

Wow, that GitHub feature to "commit suggestion" is 😍

PTAL

Copy link
Member

@jehiah jehiah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 squash?

@mreiferson
Copy link
Member Author

squashed

@mreiferson mreiferson merged commit c164c77 into nsqio:master Mar 1, 2021
@mreiferson mreiferson deleted the nsqd-client-close branch March 1, 2021 03:35
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.

3 participants