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

ZOOKEEPER-4220: Potential redundant connection attempts during leader election - branch-3.5 #1631

Closed
wants to merge 1 commit into from

Conversation

symat
Copy link
Contributor

@symat symat commented Mar 9, 2021

We have a logic in the server code, that would try to connect to an other quorum member, based
on its server ID. We identify the address assigned to this ID first based on the last committed
quorum configuration. If the connection attempt fails (or the server is not known in the
committed configuration) then we try to find the address based on the last proposed quorum
configuration. But we should do the second connection attempt, only if the address in the
last proposed configuration differs from the address in the last committed configuration.
Otherwise we would just retry to connect to the same address that failed just right before.

In the current code we have a bug, because we compare the address object references (use "!=")
instead of comparing the objects themselves (using "not equals"). In certain edge cases (e.g.
when the last proposed and last committed addresses are the same, but the address is unreachable)
this bug can lead to unnecessary retry of connection attempts. The normal behaviour would be to
mark this connection attempt to be failed and wait for e.g. the next election round or wait for
the other server to come online and initiate a connection to us.

… election

We have a logic in the server code, that would try to connect to an other quorum member, based
on its server ID. We identify the address assigned to this ID first based on the last committed
quorum configuration. If the connection attempt fails (or the server is not known in the
committed configuration) then we try to find the address based on the last proposed quorum
configuration. But we should do the second connection attempt, only if the address in the
last proposed configuration differs from the address in the last committed configuration.
Otherwise we would just retry to connect to the same address that failed just right before.

In the current code we have a bug, because we compare the address object references (use "!=")
instead of comparing the objects themselves (using "not equals"). In certain edge cases (e.g.
when the last proposed and last committed addresses are the same, but the address is unreachable)
this bug can lead to unnecessary retry of connection attempts. The normal behaviour would be to
mark this connection attempt to be failed and wait for e.g. the next election round or wait for
the other server to come online and initiate a connection to us.

Author: Mate Szalay-Beko <[email protected]>

Reviewers: Andor Molnar <[email protected]>, Damien Diederen <[email protected]>

Closes apache#1615 from symat/ZOOKEEPER-4220
@symat
Copy link
Contributor Author

symat commented Mar 9, 2021

created this PR to kick-in CI on branch 3.5 for #1615

@symat
Copy link
Contributor Author

symat commented Mar 9, 2021

CI is green, I'll go ahead and merge this, as the original PR #1615 for 3.7.0 was already reviewed by @anmolnar and @ztzg

asfgit pushed a commit that referenced this pull request Mar 9, 2021
… election

We have a logic in the server code, that would try to connect to an other quorum member, based
on its server ID. We identify the address assigned to this ID first based on the last committed
quorum configuration. If the connection attempt fails (or the server is not known in the
committed configuration) then we try to find the address based on the last proposed quorum
configuration. But we should do the second connection attempt, only if the address in the
last proposed configuration differs from the address in the last committed configuration.
Otherwise we would just retry to connect to the same address that failed just right before.

In the current code we have a bug, because we compare the address object references (use "!=")
instead of comparing the objects themselves (using "not equals"). In certain edge cases (e.g.
when the last proposed and last committed addresses are the same, but the address is unreachable)
this bug can lead to unnecessary retry of connection attempts. The normal behaviour would be to
mark this connection attempt to be failed and wait for e.g. the next election round or wait for
the other server to come online and initiate a connection to us.

Author: Mate Szalay-Beko <[email protected]>

Reviewers: Andor Molnar <[email protected]>, Damien Diederen <[email protected]>

Closes #1631 from symat/ZOOKEEPER-4220-branch-3.5
@symat symat closed this Mar 9, 2021
@ztzg
Copy link
Contributor

ztzg commented Mar 9, 2021

Thank you for taking care of it, @symat!

anmolnar pushed a commit to anmolnar/zookeeper that referenced this pull request May 21, 2024
… election

We have a logic in the server code, that would try to connect to an other quorum member, based
on its server ID. We identify the address assigned to this ID first based on the last committed
quorum configuration. If the connection attempt fails (or the server is not known in the
committed configuration) then we try to find the address based on the last proposed quorum
configuration. But we should do the second connection attempt, only if the address in the
last proposed configuration differs from the address in the last committed configuration.
Otherwise we would just retry to connect to the same address that failed just right before.

In the current code we have a bug, because we compare the address object references (use "!=")
instead of comparing the objects themselves (using "not equals"). In certain edge cases (e.g.
when the last proposed and last committed addresses are the same, but the address is unreachable)
this bug can lead to unnecessary retry of connection attempts. The normal behaviour would be to
mark this connection attempt to be failed and wait for e.g. the next election round or wait for
the other server to come online and initiate a connection to us.

Author: Mate Szalay-Beko <[email protected]>

Reviewers: Andor Molnar <[email protected]>, Damien Diederen <[email protected]>

Closes apache#1631 from symat/ZOOKEEPER-4220-branch-3.5

(cherry picked from commit 755cb27)
Change-Id: Ia1eddfb5edb8cc3f791d010301dbc1f9e2c682f5
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