You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by German Blanco <ge...@gmail.com> on 2014/03/05 15:23:52 UTC
Re: Review Request 15568: See ZOOKEEPER-1810
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15568/
-----------------------------------------------------------
(Updated March 5, 2014, 2:23 p.m.)
Review request for zookeeper, fpj and Raul Gutierrez Segales.
Changes
-------
Update with last comments
Bugs: ZOOKEEPER-1810
https://issues.apache.org/jira/browse/ZOOKEEPER-1810
Repository: zookeeper
Description
-------
See ZOOKEEPER-1810
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1574484
./src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java 1574484
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1574484
./src/java/main/org/apache/zookeeper/server/quorum/Vote.java 1574484
./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java PRE-CREATION
./src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java PRE-CREATION
./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java PRE-CREATION
./src/java/test/org/apache/zookeeper/test/FLENewEpochTest.java 1574484
./src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 1574484
./src/java/test/org/apache/zookeeper/test/FLETest.java 1574484
./src/java/test/org/apache/zookeeper/test/FLEZeroWeightTest.java 1574484
./src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 1574484
Diff: https://reviews.apache.org/r/15568/diff/
Testing
-------
Test included.
Thanks,
German Blanco
Re: Review Request 15568: See ZOOKEEPER-1810
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
> On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java, line 569
> > <https://reviews.apache.org/r/15568/diff/4/?file=510840#file510840line569>
> >
> > if this is only used for tests, shouldn't we extend this class from tests and add this there?
>
> German Blanco wrote:
> Maybe it would be a bit cleaner, but it is not the goal of ZOOKEEPER-1810 to beautify all the java classes that it touches, sorry.
My motivation wasn't to beautify the class. If it is code that is only used from tests, then you are bloating the file with it has no added benefits. Not a huge deal, except it is just more code for people to go over when they next work on that file...
> On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java, line 357
> > <https://reviews.apache.org/r/15568/diff/4/?file=510840#file510840line357>
> >
> > wouldn't this mean that we could loop forever if we got a bad state? sounds like an unknown rstate is unrecoverable, no?
>
> German Blanco wrote:
> Nope, this means that the next item in the queue will be fetch. See line aprox. 231 "response = manager.pollRecvQueue(3000, TimeUnit.MILLISECONDS);". This is the same behaviour as for a too short item in the queue, see line around 236.
> This means that if a new server state is added in the future, messages to old servers with this new server state will be ignored, instead of ... I don't know what could happen, but probably something gets broken.
Gotcha.
> On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java, line 141
> > <https://reviews.apache.org/r/15568/diff/4/?file=510844#file510844line141>
> >
> > I would bring:
> >
> > ```
> > Assert.assertTrue("State is not leading.", peer.getPeerState() == ServerState.LEADING);
> > ```
> >
> > from FLETestUtils.LEThread here, it makes what's being tested more clear. Not seeing the assertions inside the test makes it a bit harder to follow what's being tested (i think).
>
> German Blanco wrote:
> This is how it was done in branch 3.4 and it is already committed there, so I would suggest to keep it like it is, unless it breaks something and then we fix it in both branches.
Sure fair enough.
- Raul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15568/#review36241
-----------------------------------------------------------
On March 5, 2014, 3:26 p.m., German Blanco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15568/
> -----------------------------------------------------------
>
> (Updated March 5, 2014, 3:26 p.m.)
>
>
> Review request for zookeeper, fpj and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1810
> https://issues.apache.org/jira/browse/ZOOKEEPER-1810
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> See ZOOKEEPER-1810
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1574484
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java 1574484
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1574484
> ./src/java/main/org/apache/zookeeper/server/quorum/Vote.java 1574484
> ./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java PRE-CREATION
> ./src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java PRE-CREATION
> ./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java PRE-CREATION
> ./src/java/test/org/apache/zookeeper/test/FLENewEpochTest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/FLETest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/FLEZeroWeightTest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 1574484
>
> Diff: https://reviews.apache.org/r/15568/diff/
>
>
> Testing
> -------
>
> Test included.
>
>
> Thanks,
>
> German Blanco
>
>
Re: Review Request 15568: See ZOOKEEPER-1810
Posted by German Blanco <ge...@gmail.com>.
> On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java, line 357
> > <https://reviews.apache.org/r/15568/diff/4/?file=510840#file510840line357>
> >
> > wouldn't this mean that we could loop forever if we got a bad state? sounds like an unknown rstate is unrecoverable, no?
Nope, this means that the next item in the queue will be fetch. See line aprox. 231 "response = manager.pollRecvQueue(3000, TimeUnit.MILLISECONDS);". This is the same behaviour as for a too short item in the queue, see line around 236.
This means that if a new server state is added in the future, messages to old servers with this new server state will be ignored, instead of ... I don't know what could happen, but probably something gets broken.
> On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java, line 569
> > <https://reviews.apache.org/r/15568/diff/4/?file=510840#file510840line569>
> >
> > if this is only used for tests, shouldn't we extend this class from tests and add this there?
Maybe it would be a bit cleaner, but it is not the goal of ZOOKEEPER-1810 to beautify all the java classes that it touches, sorry.
> On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java, line 141
> > <https://reviews.apache.org/r/15568/diff/4/?file=510844#file510844line141>
> >
> > I would bring:
> >
> > ```
> > Assert.assertTrue("State is not leading.", peer.getPeerState() == ServerState.LEADING);
> > ```
> >
> > from FLETestUtils.LEThread here, it makes what's being tested more clear. Not seeing the assertions inside the test makes it a bit harder to follow what's being tested (i think).
This is how it was done in branch 3.4 and it is already committed there, so I would suggest to keep it like it is, unless it breaks something and then we fix it in both branches.
> On March 5, 2014, 6:12 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java, line 69
> > <https://reviews.apache.org/r/15568/diff/4/?file=510846#file510846line69>
> >
> > what i said above about moving this assertion inline with the test cases.
This is how it was done in branch 3.4 and it is already committed there, so I would suggest to keep it like it is, unless it breaks something and then we fix it in both branches.
- German
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15568/#review36241
-----------------------------------------------------------
On March 5, 2014, 3:26 p.m., German Blanco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15568/
> -----------------------------------------------------------
>
> (Updated March 5, 2014, 3:26 p.m.)
>
>
> Review request for zookeeper, fpj and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1810
> https://issues.apache.org/jira/browse/ZOOKEEPER-1810
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> See ZOOKEEPER-1810
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1574484
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java 1574484
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1574484
> ./src/java/main/org/apache/zookeeper/server/quorum/Vote.java 1574484
> ./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java PRE-CREATION
> ./src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java PRE-CREATION
> ./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java PRE-CREATION
> ./src/java/test/org/apache/zookeeper/test/FLENewEpochTest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/FLETest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/FLEZeroWeightTest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 1574484
>
> Diff: https://reviews.apache.org/r/15568/diff/
>
>
> Testing
> -------
>
> Test included.
>
>
> Thanks,
>
> German Blanco
>
>
Re: Review Request 15568: See ZOOKEEPER-1810
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15568/#review36241
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java
<https://reviews.apache.org/r/15568/#comment67187>
wouldn't this mean that we could loop forever if we got a bad state? sounds like an unknown rstate is unrecoverable, no?
./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java
<https://reviews.apache.org/r/15568/#comment67188>
if this is only used for tests, shouldn't we extend this class from tests and add this there?
./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java
<https://reviews.apache.org/r/15568/#comment67191>
nit: no need for the QuorumCnxManager.Listener var, just have:
```
cnxManagers[0].listener.start();
```
./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java
<https://reviews.apache.org/r/15568/#comment67192>
ditto.
./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java
<https://reviews.apache.org/r/15568/#comment67193>
I would bring:
```
Assert.assertTrue("State is not leading.", peer.getPeerState() == ServerState.LEADING);
```
from FLETestUtils.LEThread here, it makes what's being tested more clear. Not seeing the assertions inside the test makes it a bit harder to follow what's being tested (i think).
./src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java
<https://reviews.apache.org/r/15568/#comment67196>
nit: i would drop the comment, your code is very clean and self-explanatory.
./src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java
<https://reviews.apache.org/r/15568/#comment67195>
nit: ditto wrt to listener var not needed.
./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java
<https://reviews.apache.org/r/15568/#comment67197>
what i said above about moving this assertion inline with the test cases.
- Raul Gutierrez Segales
On March 5, 2014, 3:26 p.m., German Blanco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15568/
> -----------------------------------------------------------
>
> (Updated March 5, 2014, 3:26 p.m.)
>
>
> Review request for zookeeper, fpj and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1810
> https://issues.apache.org/jira/browse/ZOOKEEPER-1810
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> See ZOOKEEPER-1810
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1574484
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java 1574484
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1574484
> ./src/java/main/org/apache/zookeeper/server/quorum/Vote.java 1574484
> ./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java PRE-CREATION
> ./src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java PRE-CREATION
> ./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java PRE-CREATION
> ./src/java/test/org/apache/zookeeper/test/FLENewEpochTest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/FLETest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/FLEZeroWeightTest.java 1574484
> ./src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 1574484
>
> Diff: https://reviews.apache.org/r/15568/diff/
>
>
> Testing
> -------
>
> Test included.
>
>
> Thanks,
>
> German Blanco
>
>
Re: Review Request 15568: See ZOOKEEPER-1810
Posted by German Blanco <ge...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15568/
-----------------------------------------------------------
(Updated March 5, 2014, 3:26 p.m.)
Review request for zookeeper, fpj and Raul Gutierrez Segales.
Bugs: ZOOKEEPER-1810
https://issues.apache.org/jira/browse/ZOOKEEPER-1810
Repository: zookeeper
Description
-------
See ZOOKEEPER-1810
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1574484
./src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java 1574484
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1574484
./src/java/main/org/apache/zookeeper/server/quorum/Vote.java 1574484
./src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java PRE-CREATION
./src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java PRE-CREATION
./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java PRE-CREATION
./src/java/test/org/apache/zookeeper/test/FLENewEpochTest.java 1574484
./src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 1574484
./src/java/test/org/apache/zookeeper/test/FLETest.java 1574484
./src/java/test/org/apache/zookeeper/test/FLEZeroWeightTest.java 1574484
./src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 1574484
Diff: https://reviews.apache.org/r/15568/diff/
Testing
-------
Test included.
Thanks,
German Blanco