search

nsqio / go-nsq

The official Go package for NSQ
MIT License
2.59k stars 444 forks source link

command: register support topic/channel state #305

Open maplessssy opened 4 years ago

maplessssy commented 4 years ago

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

maplessssy commented 4 years ago

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

ploxiln commented 4 years ago

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 commented 4 years ago

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

mreiferson commented 4 years ago

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 commented 4 years ago

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 .

maplessssy commented 4 years ago

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

mreiferson commented 4 years ago

this is looking good, thanks for your patience!