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: expand lookupd DNS names #897

Closed
wants to merge 1 commit into from
Closed

nsqd: expand lookupd DNS names #897

wants to merge 1 commit into from

Conversation

htdvisser
Copy link

This Pull Requests fixes #646 by resolving the DNS for --lookupd-tcp-address and adding each result separately.

Notes:

  • As it's not easy to mock DNS resolution, I did not add any additional tests.
  • It might be nice to implement something similar for the HTTP endpoints, but the logic implemented in this PR would only partly work, as HTTP requires the correct Host header (to make vhosts work correctly, for example)

-- ready for review

@mreiferson
Copy link
Member

@htdvisser thanks for your contribution! ❤️ 💯

After scanning the diff, I think we're going to need to update this logic too https://github.com/nsqio/nsq/blob/master/nsqd/lookup.go#L151-L165

@htdvisser
Copy link
Author

I agree. But then we have to keep in mind that two DNS names might resolve to the same IP, and simply removing all IPs the name resolves to, might remove more than we actually want to remove.

@mreiferson
Copy link
Member

My expectation would be that if I'm dynamically updating the list of configured nsqlookupd addresses then I would only want to connect to the resolved IPs of the newly supplied addresses. Implementation wise, I think that means that we should:

  1. resolve the IPs for the newly supplied addresses
  2. diff that list with the list of already connected IPs
  3. close/remove the ones that are no longer present
  4. connect to the ones that are new

Also, somewhat related, I think we need to update reconnection logic too. Imagine the case where the resolved IPs change but the configured addresses do not. We shouldn't continue to attempt to reconnect to an old resolved IP, it should trigger resolution of the configured addresses to a list of IPs that are processed the same way as the steps above.

@htdvisser
Copy link
Author

Then we should probably periodically re-resolve the names in order to "discover" new/changed IPs

@mreiferson
Copy link
Member

Yep, something like that. Interested in seeing how that comes out?

@mreiferson mreiferson changed the title nsqd: Expand lookupd DNS names nsqd: expand lookupd DNS names May 21, 2017
@htdvisser
Copy link
Author

After working on this a bit, I'm not completely sure anymore if it's such a good idea to periodically re-resolve the DNS. Changes in behavior of nsqd will become "magic", as they might happen at any time (because we resolve periodically) and might seem random because the DNS server can change or break at any time.

In my opinion configuration changes like this should be user initiated, so resolving DNS should only happen if the user changes the config or sends some kind of "reload" signal.

On the other hand, if we work with these "manual" reloads, then what's the point of resolving the DNS names in the first place, as then you could just as well enter the IP addresses directly.

Closing this PR for now.

@htdvisser htdvisser closed this May 26, 2017
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.

nsqd: expand DNS records for "service discovery"
2 participants