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

Cannot resolve .. [Errno -2] Name or service not known #631

Merged
merged 3 commits into from
Jan 20, 2021

Conversation

ksarabu1
Copy link
Contributor

@ksarabu1 ksarabu1 commented Dec 8, 2020

Fixes #

Why is this needed?

Zookeeper is running from Docker containers and the service being resolved using Consul. When there's a network issue, the call to socket.getaddrinfo in method _expand_client_hosts causing it to return the error Cannot resolve .. [Errno -2] Name or service not known. This is causing retry to throwback STOP_CONNECTING and it stops retrying as result.

Proposed Changes

Change: Raise ForceRetryError if method _expand_client_hosts returns empty host/ports and client.hosts has only one host to resolve.

        # Check for an empty hostlist, indicating none resolved
        if len(host_ports) == 0:
            if len(self.client.hosts) == 1:
                raise ForceRetryError('Reconnecting')
            return STOP_CONNECTING

Does this PR introduce any breaking change?

n/a

@@ -543,6 +543,8 @@ def _connect_loop(self, retry):

# Check for an empty hostlist, indicating none resolved
if len(host_ports) == 0:
if len(self.client.hosts) == 1:
Copy link
Member

Choose a reason for hiding this comment

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

Would you be willing to explain the reason for this additional condition? Looking at the code, I would have replaced the return STOP_CONNECTING with a raise ForceRetryError('No host resolved, reconnecting') but you may have seen something I am missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I was just trying to keep the behavior to default when multiple hosts are used and if they all fail to resolve. I would also prefer to keep it simple and raise retry in all cases. Will update.

@StephenSorriaux
Copy link
Member

StephenSorriaux commented Dec 8, 2020

Thans for this PR.

Overall I am fine with the idea behind the change, it makes sense to trigger the retry policy in case hosts are not resolved.

@ceache
Copy link
Contributor

ceache commented Dec 9, 2020

I second @StephenSorriaux's feedback. The idea is good but I am confused by the additional check on a single host in the list.

Copy link
Member

@StephenSorriaux StephenSorriaux left a comment

Choose a reason for hiding this comment

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

Thank you.

@ksarabu1
Copy link
Contributor Author

ksarabu1 commented Jan 19, 2021

@ceache Can you please review. Thanks

Copy link
Contributor

@ceache ceache left a comment

Choose a reason for hiding this comment

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

Thanks!

@ksarabu1
Copy link
Contributor Author

Thank you. Can you please merge the PR? Thanks in advance.

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.

3 participants