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: start Topic messagePump separately from create #1053

Merged
merged 2 commits into from
Jul 10, 2018

Conversation

ploxiln
Copy link
Member

@ploxiln ploxiln commented Jul 7, 2018

with new Topic.Start()

and make Topic.Pause() and Topic.GetChannel() no longer block
on the notification channels for Topic.messagePump()

and simplify some calls to WaitGroupWrapper.Wrap()

follow-up to #1050 cc @mreiferson
possibly better, but does change the behavior of Pause() and GetChannel() slightly ...

@mreiferson
Copy link
Member

mreiferson commented Jul 8, 2018

I like the way this is coming out, thanks.

and make Topic.Pause() and Topic.GetChannel() no longer block on the notification channels for Topic.messagePump()

I was wondering if this would cause random timing-related test failures, looks like it hasn't. However, I'm a little concerned about the subtlety of this change.

For Pause(), I'm thinking that for topic creation code paths, GetTopic and NewTopic should really take paused as a parameter in their signatures. This way we wouldn't have to call Paused immediately after to set appropriate state (code would look a _little_cleaner too, I think). I realize we'll have to change 1000 places in tests we call GetTopic, but I think I'd prefer that over changing the behavior of the notification.

I don't have any great ideas for GetChannel though, what do you think?

@ploxiln ploxiln force-pushed the topic_start branch 2 times, most recently from 4db8a66 to c635ce9 Compare July 9, 2018 02:01
@ploxiln
Copy link
Member Author

ploxiln commented Jul 9, 2018

I don't like the idea of adding a paused parameter to GetTopic() (only to be used if it creates a new topic?)

However, I came up with another design. Topic.messagePump() is started by NewTopic() as before, but now it has a pre-start-loop to recv on pauseChan and channelUpdateChan, and which is ended via a new startChan.

@mreiferson
Copy link
Member

Nice, I like it. LGTM, squash?

@ploxiln
Copy link
Member Author

ploxiln commented Jul 9, 2018

Will squash and do some local testing ...

ploxiln added 2 commits July 9, 2018 19:45
Add Topic.Start() method, call after all initial channels are added
to topic. This way Topics can be created un-paused, like they always
have been until very recently.

Topic.messagePump() does not pass messages until after being
notified by Start() via the new startChan.
@ploxiln
Copy link
Member Author

ploxiln commented Jul 10, 2018

updated

@mreiferson mreiferson merged commit 6ab87aa into nsqio:master Jul 10, 2018
@ploxiln
Copy link
Member Author

ploxiln commented Jul 10, 2018

thanks for the review!

@ploxiln ploxiln deleted the topic_start branch August 1, 2018 20:48
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