-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: add topology region/zone aware message consumption #1301
base: master
Are you sure you want to change the base?
Conversation
I think this is ready for a first round of review @mreiferson @ploxiln (along w/ it's pair nsqio/go-nsq#312 Some items to decide on -
Once this is squared away and we are happy with it i'll follow up with documentation PR's and exposing region/zone in nsqadmin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some ideas, for what I think are minor simplifications that leave the overall design the same. Overall looks good to me.
apps/nsqd/options.go
Outdated
@@ -120,6 +120,8 @@ func nsqdFlagSet(opts *nsqd.Options) *flag.FlagSet { | |||
flagSet.Var(&lookupdTCPAddrs, "lookupd-tcp-address", "lookupd TCP address (may be given multiple times)") | |||
flagSet.Duration("http-client-connect-timeout", opts.HTTPClientConnectTimeout, "timeout for HTTP connect") | |||
flagSet.Duration("http-client-request-timeout", opts.HTTPClientRequestTimeout, "timeout for HTTP request") | |||
flagSet.String("topology-region", opts.TopologyRegion, "A region represents a larger domain, made up of one or more zones") | |||
flagSet.String("topology-zone", opts.TopologyZone, "A zone represents a logical failure domain") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably mention something about "for preferring closer consumer"
f390edc
to
96adf24
Compare
91517a4
to
fa8d2b4
Compare
2b90b29
to
42d28f5
Compare
88161cb
to
af1edde
Compare
af1edde
to
4631064
Compare
e93d2b5
to
b84cfe2
Compare
83e5939
to
d0cd516
Compare
8f51ed1
to
00fcfa9
Compare
@jehiah @ploxiln @mreiferson This PR is now officially ready for review - in conjunction with nsqio/go-nsq#312 and nsqio/nsqio.github.io#89. We have written up an experience report based on our observations running these changes for 2 months in our production environment - it can be found here. It also lays out the changes to nsqadmin. Please let me know if you have any other questions! Thanks in advance! |
Co-authored-by: Jehiah Czebotar <jehiah@gmail.com>
Co-authored-by: Jehiah Czebotar <jehiah@gmail.com>
@jehiah hello! what's the plan here for actually landing this? Are y'all waiting on me to review everything 😏? |
@mreiferson sorry for the radio silence on this - I think we are ready to give more attention to this in Q4 so if you have any comments on this PR (or the related go-nsq changes) in the next few weeks please share them otherwise the topology changes are ready to land and we'll do that next month. I think the general plan is to still land this behind the feature flag and get them in a 1.4 release and consider removing the FF in 1.5. We have been happy with this running at Bitly for a while now, but i'm keeping an eye on kubernetes/enhancements#4747 as it will make adopting in a K8s environment easier. |
This updates nsqd to have a
--topology-region
and--topology-zone
config argument and prefer sending messages to same-zone and same-region consumers as proposed in #1300