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: nsqlookupd connect callback simpler, more robust #1008

Merged
merged 2 commits into from
May 12, 2018

Conversation

ploxiln
Copy link
Member

@ploxiln ploxiln commented Mar 6, 2018

Remove syncTopicChan and do the initial REGISTER commands in
the lookupPeer connectCallback(). This should be mostly
equivalent, just with less indirection.

Close() the lookupPeer in all error cases. It will attempt to
re-connect with the next Ping command.

This stuff was brought up in #993 - consider this an un-tested proposal. Please question/comment :)

@mreiferson
Copy link
Member

I can't think of any reason why this would be a bad idea!

@mreiferson
Copy link
Member

... mostly this is because I can no longer remember why we wrote it with such indirection. My assumption is that it's just really old Go code of ours that had way too much use of goroutines and channels 😀

Remove syncTopicChan and do the initial REGISTER commands in
the lookupPeer connectCallback(). This should be mostly
equivalent, just with less indirection.

Close() the lookupPeer in all error cases. It will attempt to
re-connect with the next Ping command.
@ploxiln ploxiln force-pushed the nsqd_lookup_refact branch from dae1094 to 81ec639 Compare April 13, 2018 16:35
@ploxiln
Copy link
Member Author

ploxiln commented Apr 13, 2018

Rebased and squashed in the little fixups. I'm going to deploy this out to my infrastructure to let it stew for a bit (but honestly my nsq cluster is not very big or stressed).

@ploxiln
Copy link
Member Author

ploxiln commented May 9, 2018

I've been running this on a few servers for about a week, using this test-integration branch: https://github.com/ploxiln/nsq/commits/test_201804

@mreiferson mreiferson merged commit 9dbd684 into nsqio:master May 12, 2018
@ploxiln ploxiln deleted the nsqd_lookup_refact branch June 2, 2018 17: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.

2 participants