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

NSQLookupd support for nsq_consumer #3215

Merged
merged 2 commits into from
Sep 25, 2017
Merged

NSQLookupd support for nsq_consumer #3215

merged 2 commits into from
Sep 25, 2017

Conversation

ljagiello
Copy link
Contributor

@ljagiello ljagiello commented Sep 10, 2017

This PR adds support for NSQLookupd in nsq_consumer.

Why:
Existing solution where nsq_consumer reads only from a single NSQD server limits ability to take advantages from distributed nature of NSQ.

NSQ godocs suggest to use nsqlookupd as a prefered method for connection (https://godoc.org/github.com/nsqio/go-nsq#Consumer.ConnectToNSQD):

It is recommended to use ConnectToNSQLookupd so that topics are discovered automatically. This method is useful when you want to connect to a single, local, instance.

nsqlookupd is the daemon that manages topology information. Clients query nsqlookupd to discover nsqd producers for a specific topic and nsqd nodes broadcasts topic and channel information.

With nsqLookupd support nsq_consumer will detect all avaiable nsqd producers and connects to all.

Additionally sync with the latest stable release of go-nsq (https://github.com/nsqio/go-nsq/blob/master/ChangeLog.md#107---2017-08-04)

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

This PR adds support for NSQLookupd in `nsq_consumer`.

Why:
Existing solution where `nsq_consumer` reads only from a single
NSQD server limits ability to take advantages from distributed
nature of NSQ.

NSQ godocs suggest to use `nsqlookupd` as a prefered method for
connection (https://godoc.org/github.com/nsqio/go-nsq#Consumer.ConnectToNSQD):
```
It is recommended to use ConnectToNSQLookupd so that topics are
discovered automatically. This method is useful when you want
to connect to a single, local, instance.
```

`nsqlookupd` is the daemon that manages topology information. Clients
query nsqlookupd to discover nsqd producers for a specific topic and
nsqd nodes broadcasts topic and channel information.

With `nsqLookupd` support `nsq_consumer` will detect all avaiable
nsqd producers and connects to all.
@ljagiello
Copy link
Contributor Author

Tests failed on kafka_consumer_integration_test.go not related with this PR.

@danielnelson
Copy link
Contributor

One concern with this change is that since it changes the type of the server field it will require users to update their config file. One way this could be avoided is by changing the new nsqlookupd boolean to be an array of nsqlookupd instances to connect to, but this method would not be very clear that you should choose either the server or nsqlookupd option and not both.

Looking over the nsq GoDoc, it looks like it is possible to both connect to a specific nsqd and at the same time be connected to nsqlookupd doing discovery, does it make any sense to combine these options? Perhaps someone would connect to a localhost instance while still being able to fallback to discovered instances?

Then we could have something like:

[[inputs.nsq]]
  ## Server option still works but is deprecated, we just prepend it to the nsqd array.
  # server = "localhost"
  nsqd = ["localhost"]
  nsqlookupd = ["localhost"]

and the plugin does both:

n.consumer.ConnectToNSQLookupds(n.NSQDLookupds)
n.consumer.ConnectToNSQDs(n.NSQDs)

@ljagiello
Copy link
Contributor Author

@danielnelson sounds like a good idea, I will test that approach and provide necessary changes to PR.

@ljagiello
Copy link
Contributor Author

@danielnelson now it's possible to use old setup or new setup or mix of 2.

@danielnelson danielnelson added this to the 1.5.0 milestone Sep 25, 2017
@danielnelson danielnelson merged commit a4b8805 into influxdata:master Sep 25, 2017
@danielnelson
Copy link
Contributor

Thank you! Will be included in 1.5.0.

@ljagiello ljagiello deleted the nsqdlookupds branch September 25, 2017 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants