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

command: register support topic/channel state #305

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

maplessssy
Copy link

command: register support topic/channel state
see nsq issue:1199

@maplessssy
Copy link
Author

hi, anyone can help to take a look at this?

@ploxiln
Copy link
Member

ploxiln commented Aug 7, 2020

Yeah, yeah, I saw it.

The interesting detail, is how current nsqlookupd will handle receiving more params. Backwards compatibility is needed while rolling out new versions. It looks like current nsqlookupd will ignore extra params, which is good.

There is another interesting detail: currently, topics/channels are only registered once, per connection to nsqlookupd, I think. But to use this, they will need to be re-registered whenever pause state changes. That's a possibly interesting change. There's also the fact that the topic pause state is sent redundantly when every channel state is sent ... not necessarily a problem for performance/efficiency, but rather confusion about what to do with multiple potentially conflicting reports. Always take the last topic state reported, even if that was just part of a channel state report/registration, I guess?

I think it will all work out fine, but it is good to clearly see how the details will look in the other projects, before finalizing this design. So, I would open up a pull request on the main nsq repo, modifying nsqd and nsqlookupd to use this new functionality, so we can see how that looks. You can try to temporarily modify the go.mod to use this commit of go-nsq so your nsq PR is able to build and test.

@maplessssy
Copy link
Author

maplessssy commented Aug 8, 2020

@ploxiln I hava opend up a pull request on the main nsq repo, see nsq pr. : )

@mreiferson
Copy link
Member

Building on @ploxiln's feedback, I think the underlying problem with this implementation is that it's not a "registration". Perhaps we need a new command that represents a topic/channel "state change"?

@maplessssy
Copy link
Author

Building on @ploxiln's feedback, I think the underlying problem with this implementation is that it's not a "registration". Perhaps we need a new command that represents a topic/channel "state change"?

I think a new command is good, it's more clearly that topic and channel state change can delivery more independently. @ploxiln and also resolve the topic pause state is sent redundantly when every channel state is sent .

command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
@maplessssy
Copy link
Author

@mreiferson This PR is ready for review. PTAL.
/cc @ploxiln

command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
@mreiferson
Copy link
Member

this is looking good, thanks for your patience!

Copy link
Member

@mreiferson mreiferson left a comment

Choose a reason for hiding this comment

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

LGTM, but let's not merge this until we're done with nsqio/nsq#1274

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

Successfully merging this pull request may close these issues.

3 participants