Skip to content
This repository was archived by the owner on Jul 21, 2021. It is now read-only.

Implement ZOOKEEPER-1576 for golang - #188

Open
juvenzhu wants to merge 1 commit into
samuel:masterfrom
juvenzhu:master
Open

Implement ZOOKEEPER-1576 for golang#188
juvenzhu wants to merge 1 commit into
samuel:masterfrom
juvenzhu:master

Conversation

@juvenzhu

@juvenzhu juvenzhu commented Mar 5, 2018

Copy link
Copy Markdown

When one node in the zk connection string is down, the current behavior of this library is to fail the connection. In that case, Quorum does maintain but zkclient using this library will not be able to connect to zk.

This pull request implements Zookeeper-1576. As far as one node in the zk connection string is alive, zkclient should still able to connect.

@coveralls

coveralls commented Mar 5, 2018

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 80.07% when pulling 519c719 on juvenzhu:master into c4fab1a on samuel:master.

Comment thread zk/dnshostprovider.go Outdated
host, port, err := net.SplitHostPort(server)
if err != nil {
return err
// If one node is down, Init should still continue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why this change? in this case the node isn't down - instead it's a configuration error. shouldn't this fail-fast?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good call. If it's a configuration error, we should fail fast here

Comment thread zk/dnshostprovider.go Outdated
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: needless whitespace

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

will update

Comment thread zk/dnshostprovider.go
addrs, err := lookupHost(host)
if err != nil {
return err
// If one node is down, Init should still continue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like this change

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks @jdef for reviewing. I have updated this diff

@juvenzhu

juvenzhu commented Mar 6, 2018

Copy link
Copy Markdown
Author

rebase the pull request

@jdef

jdef commented Mar 7, 2018

Copy link
Copy Markdown
Contributor

Prior to this change, problems resolving a host would error out. One of the consequences of this change is that if you specify 3 hosts, but there's an error resolving 1 of them, then the ZK connection will never attempt to connect to it. I'm not convinced that's desirable. For example, it's probably better to defer resolution of that host and try again later -- vs permanently forgetting about it.

@juvenzhu

juvenzhu commented Mar 7, 2018

Copy link
Copy Markdown
Author

Thanks for the review!

If there is a host down, we can add the down host into the dnsprovider server list and let the connect loop to automatically discover it when it's up again. That down host is just not used during Init.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants