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-2959: ignore accepted epoch and LEADERINFO ack from observers #500

Closed
wants to merge 1 commit into
base: branch-3.4
from

Conversation

Projects
None yet
5 participants
@lavacat
Contributor

lavacat commented Apr 8, 2018

https://issues.apache.org/jira/browse/ZOOKEEPER-2959

  • add getVotingView check for id in getEpochToPropose and waitForEpochAck
  • refactor waitForNewLeaderAck to use getVotingView
  • unit tests

credit: Xiang Yongqiang (https://github.com/xyq000) for original PR and reporting the issue

@shralex

This comment has been minimized.

Show comment
Hide comment
@shralex

shralex Apr 8, 2018

Contributor

I'm +1. Thanks Bogdan for making the PR.

Contributor

shralex commented Apr 8, 2018

I'm +1. Thanks Bogdan for making the PR.

@anmolnar

anmolnar suggested changes Apr 12, 2018 edited

The fix looks good to me, I would only like to suggest a few nitpicks in the tests.
The init and finally parts of these tests are redundant, so I think it'd be great if we can move these new tests to a separate test file and add proper setup() and teardown() methods.

On the top of that, existing tests follow the same pattern as far as I can see, so why not we just refactor all of them to take advantage of junit before-after mechanism?

@anmolnar

This comment has been minimized.

Show comment
Hide comment
@anmolnar

anmolnar Apr 12, 2018

Contributor

Given that this change affects leader election I think it'd be very beneficial if @fpj could take a look by any chance.

Contributor

anmolnar commented Apr 12, 2018

Given that this change affects leader election I think it'd be very beneficial if @fpj could take a look by any chance.

@lavacat

This comment has been minimized.

Show comment
Hide comment
@lavacat

lavacat Apr 12, 2018

Contributor

Moved these 3 new tests into new class - LeaderWithObserverTest. Had to make createLeader and createQuorumPeer 'public static' in Zab1_0Test. Happy to refactor into common base class

Contributor

lavacat commented Apr 12, 2018

Moved these 3 new tests into new class - LeaderWithObserverTest. Had to make createLeader and createQuorumPeer 'public static' in Zab1_0Test. Happy to refactor into common base class

@anmolnar

This comment has been minimized.

Show comment
Hide comment
@anmolnar

anmolnar Apr 12, 2018

Contributor

@lavacat I think either moving these methods/classes to a base class or creating a separate ZabUtils makes sense in this case to get cleaner code. Not sure which one is better. If they don't have state, separate class is neater.

Contributor

anmolnar commented Apr 12, 2018

@lavacat I think either moving these methods/classes to a base class or creating a separate ZabUtils makes sense in this case to get cleaner code. Not sure which one is better. If they don't have state, separate class is neater.

@lavacat

This comment has been minimized.

Show comment
Hide comment
@lavacat

lavacat Apr 13, 2018

Contributor

@anmolnar added ZabUtils

Contributor

lavacat commented Apr 13, 2018

@anmolnar added ZabUtils

@anmolnar

@lavacat +1 cool!

ZOOKEEPER-2959: ignore accepted epoch and LEADERINFO ack from observers
https://issues.apache.org/jira/browse/ZOOKEEPER-2959
- added getVotingView check for id in getEpochToPropose and waitForEpochAck
- refactored waitForNewLeaderAck to use getVotingView
- unit tests
- refactored common test helpers into ZabUtils

credit: Xiang Yongqiang (https://github.com/xyq000) for original PR and reporting the issue

@lavacat lavacat closed this May 22, 2018

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