You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@zookeeper.apache.org by Daniel Peon <da...@ericsson.com> on 2013/12/11 09:36:45 UTC

Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/
-----------------------------------------------------------

Review request for zookeeper, German Blanco and fpj.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs
-----

./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1541529
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1541529
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1541529
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 1541529
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1541529

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/#review30347
-----------------------------------------------------------



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58077>

    nit: extra newline



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58078>

    nit: extra newline



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58075>

    nit: close implies flush, see http://docs.oracle.com/javase/7/docs/api/java/io/OutputStreamWriter.html#close%28%29



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58076>

    ditto, flush is not needed. 



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58079>

    nit: extra newline



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58080>

    nit: extra newline



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58081>

    nit: extra newline



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58082>

    nit: extra newline



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58087>

    nit: i don't think the LOG statement needs a comment, you can probably drop it and it would still be clear. 



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58084>

    nit: make these lines < 80 cols.



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58085>

    nit: make these lines < 80 cols.



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment58083>

    nit: you can use String.format instead of concatenating strings.
    
    [1] http://docs.oracle.com/javase/7/docs/api/java/lang/String.html#format%28java.lang.String,%20java.lang.Object...%29


- Raul Gutierrez Segales


On Dec. 13, 2013, 9:13 a.m., Daniel Peon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15753/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2013, 9:13 a.m.)
> 
> 
> Review request for zookeeper, German Blanco and fpj.
> 
> 
> Bugs: ZOOKEEPER-1814
>     https://issues.apache.org/jira/browse/ZOOKEEPER-1814
> 
> 
> Repository: zookeeper
> 
> 
> Description
> -------
> 
> See Zookeeper-1814.
> 
> 
> Diffs
> -----
> 
>   ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1550368 
>   ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1550368 
>   ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1550368 
>   ./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/15753/diff/
> 
> 
> Testing
> -------
> 
> New test case checking that the parameter maxNotificationInterval can be configurable.
> 
> The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.
> 
> 
> File Attachments
> ----------------
> 
> ZOOKEEPER-1814.patch
>   https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch
> 
> 
> Thanks,
> 
> Daniel Peon
> 
>

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/#review59155
-----------------------------------------------------------

Ship it!


Ship It!

- Daniel Peon


On Oct. 30, 2014, 8:01 a.m., Daniel Peon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15753/
> -----------------------------------------------------------
> 
> (Updated Oct. 30, 2014, 8:01 a.m.)
> 
> 
> Review request for zookeeper, German Blanco and fpj.
> 
> 
> Bugs: ZOOKEEPER-1814
>     https://issues.apache.org/jira/browse/ZOOKEEPER-1814
> 
> 
> Repository: zookeeper
> 
> 
> Description
> -------
> 
> See Zookeeper-1814.
> 
> 
> Diffs
> -----
> 
>   ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1635167 
>   ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1635167 
>   ./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java 1635167 
>   ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1635167 
>   ./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/15753/diff/
> 
> 
> Testing
> -------
> 
> New test case checking that the parameter maxNotificationInterval can be configurable.
> 
> The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.
> 
> 
> File Attachments
> ----------------
> 
> ZOOKEEPER-1814.patch
>   https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch
> 
> 
> Thanks,
> 
> Daniel Peon
> 
>

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Oct. 30, 2014, 8:01 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Corrected whitespaces and tabs.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1635167
./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1635167
./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java 1635167
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1635167
./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Oct. 30, 2014, 7:56 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Patch has been adapted to trunk branch with the modifications since the patch was created. Otherwise, the previous patch cannot be applied.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1634886
./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1634886
./src/java/test/org/apache/zookeeper/server/quorum/FLETestUtils.java 1634886
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1634886
./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated April 2, 2014, 9:50 a.m.)

Review request for zookeeper, German Blanco and fpj.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs
-----

./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1583935
./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1583935
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1583935
./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/
-----------------------------------------------------------

(Updated April 2, 2014, 9:49 a.m.)


Review request for zookeeper, German Blanco and fpj.


Changes
-------

Deleted again spaces at the end of some lines.


Bugs: ZOOKEEPER-1814
    https://issues.apache.org/jira/browse/ZOOKEEPER-1814


Repository: zookeeper


Description
-------

See Zookeeper-1814.


Diffs (updated)
-----

  ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1583935 
  ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1583935 
  ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1583935 
  ./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/15753/diff/


Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.


File Attachments
----------------

ZOOKEEPER-1814.patch
  https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch


Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/
-----------------------------------------------------------

(Updated April 2, 2014, 9:47 a.m.)


Review request for zookeeper, German Blanco and fpj.


Changes
-------

Deleted space at the end of a line.


Bugs: ZOOKEEPER-1814
    https://issues.apache.org/jira/browse/ZOOKEEPER-1814


Repository: zookeeper


Description
-------

See Zookeeper-1814.


Diffs (updated)
-----

  ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1583935 
  ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1583935 
  ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1583935 
  ./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/15753/diff/


Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.


File Attachments
----------------

ZOOKEEPER-1814.patch
  https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch


Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated April 2, 2014, 9:41 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Patch corrected. Previous patch cannot be apply to the latest trunk version.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/
-----------------------------------------------------------

(Updated April 2, 2014, 7:52 a.m.)


Review request for zookeeper, German Blanco and fpj.


Changes
-------

Removed extra space at the end of a line.


Bugs: ZOOKEEPER-1814
    https://issues.apache.org/jira/browse/ZOOKEEPER-1814


Repository: zookeeper


Description
-------

See Zookeeper-1814.


Diffs (updated)
-----

  ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1550368 
  ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1550368 
  ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1550368 
  ./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/15753/diff/


Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.


File Attachments
----------------

ZOOKEEPER-1814.patch
  https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch


Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated April 2, 2014, 7:50 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Documentation has been updated following the comments for this JIRA. It was a poor documentation of the new introduced configurable parameter.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1550368
./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1550368
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1550368
./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Dec. 16, 2013, 8:41 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Corrected extra spaces

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Dec. 16, 2013, 8:34 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Corrected suggested changes.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Dec. 13, 2013, 9:13 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

New spaces introduced by text editor have been corrected.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Dec. 13, 2013, 9:11 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Suggested changes applied.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/#review30267
-----------------------------------------------------------



./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java
<https://reviews.apache.org/r/15753/#comment57933>

    (commented on the JIRA but commenting here as well to make it easier to fix/follow-up).
    
    This can just be:
    
    maxNotificationInterval = Integer.getInteger("zookeeper.maxFleNotificationInterval", 60000);



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment57934>

    Nit: for all the LOG.info/LOG.debug statements, I think it's cleaner to use string extrapolation as opposed to concatenating strings. i.e.: instead of:
    
    LOG.info("Two equal notification intervals measured [" + counter + " of "
    +                                + numEqualIntervalsNeeded + "]");
    
    just:
    
    LOG.info("Two equal notification intervals measured [{} of {}]", counter, numEqualIntervalsNeeded);
    
    



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment57935>

    maxFleNotificationInterval = Integer.getInteger("zookeeper.maxFleNotificationInterval");
    
    you probably want to make "zookeeper.maxFleNotificationInterval" a public constant somewhere. 



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment57936>

    simpler: Assert.assertFalse("Should not be alive", thread.isAlive());


- Raul Gutierrez Segales


On Dec. 12, 2013, 5:12 p.m., Daniel Peon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15753/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2013, 5:12 p.m.)
> 
> 
> Review request for zookeeper, German Blanco and fpj.
> 
> 
> Bugs: ZOOKEEPER-1814
>     https://issues.apache.org/jira/browse/ZOOKEEPER-1814
> 
> 
> Repository: zookeeper
> 
> 
> Description
> -------
> 
> See Zookeeper-1814.
> 
> 
> Diffs
> -----
> 
>   ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1550368 
>   ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1550368 
>   ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1550368 
>   ./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/15753/diff/
> 
> 
> Testing
> -------
> 
> New test case checking that the parameter maxNotificationInterval can be configurable.
> 
> The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.
> 
> 
> File Attachments
> ----------------
> 
> ZOOKEEPER-1814.patch
>   https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch
> 
> 
> Thanks,
> 
> Daniel Peon
> 
>

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Dec. 12, 2013, 5:12 p.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Updated because a wrong indentation in one of the lines.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Dec. 12, 2013, 5:07 p.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Uploaded the patch with recommended changes. Additionally the notifications interval for leader election was reduced during the test case.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by German Blanco <ge...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/#review30254
-----------------------------------------------------------



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment57918>

    Please reindent this file.
    4 spaces, not tabs.
    These indentation rules must be in the How To Contribute guidelines somewhere.



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment57919>

    The comment is hard to understand ?



./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java
<https://reviews.apache.org/r/15753/#comment57920>

    It seems to be possible to implement this with a Latch, instead of waiting in a loop. Could you please change it?


In general this looks very good to me :-)

- German Blanco


On Dec. 12, 2013, 8:53 a.m., Daniel Peon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15753/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2013, 8:53 a.m.)
> 
> 
> Review request for zookeeper, German Blanco and fpj.
> 
> 
> Bugs: ZOOKEEPER-1814
>     https://issues.apache.org/jira/browse/ZOOKEEPER-1814
> 
> 
> Repository: zookeeper
> 
> 
> Description
> -------
> 
> See Zookeeper-1814.
> 
> 
> Diffs
> -----
> 
>   ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1550368 
>   ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1550368 
>   ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1550368 
>   ./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/15753/diff/
> 
> 
> Testing
> -------
> 
> New test case checking that the parameter maxNotificationInterval can be configurable.
> 
> The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.
> 
> 
> File Attachments
> ----------------
> 
> ZOOKEEPER-1814.patch
>   https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch
> 
> 
> Thanks,
> 
> Daniel Peon
> 
>

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Dec. 12, 2013, 8:53 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

In order to avoid a new warning detected during the regression, I upload the Diff r5.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Dec. 12, 2013, 8:42 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Spaces and tabs corrected (it was still a missing space)

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/#review30248
-----------------------------------------------------------



File Attachment: ZOOKEEPER-1814.patch - ZOOKEEPER-1814.patch
<https://reviews.apache.org//r/15753/#fcomment5>
    It seems to be a wrong patch. I've added a new version.

- Daniel Peon


On Dec. 12, 2013, 8:38 a.m., Daniel Peon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15753/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2013, 8:38 a.m.)
> 
> 
> Review request for zookeeper, German Blanco and fpj.
> 
> 
> Bugs: ZOOKEEPER-1814
>     https://issues.apache.org/jira/browse/ZOOKEEPER-1814
> 
> 
> Repository: zookeeper
> 
> 
> Description
> -------
> 
> See Zookeeper-1814.
> 
> 
> Diffs
> -----
> 
>   ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1550368 
>   ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1550368 
>   ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1550368 
>   ./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/15753/diff/
> 
> 
> Testing
> -------
> 
> New test case checking that the parameter maxNotificationInterval can be configurable.
> 
> The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.
> 
> 
> File Attachments
> ----------------
> 
> ZOOKEEPER-1814.patch
>   https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch
> 
> 
> Thanks,
> 
> Daniel Peon
> 
>

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Dec. 12, 2013, 8:38 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

Spaces and tabs corrected.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.

(Updated Dec. 12, 2013, 8:30 a.m.)

Review request for zookeeper, German Blanco and fpj.

Changes
-------

I created a new patch from the latest revision (1550368). It seems that the previous one didn't match to the changes that I made (it appeared some modified files that I didn't modified actually). Additionally I performed the 'svn add' operation on the test file. It should appear a new file (src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java).

Thanks and regards.

Bugs: ZOOKEEPER-1814
https://issues.apache.org/jira/browse/ZOOKEEPER-1814

Repository: zookeeper

Description
-------

See Zookeeper-1814.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/15753/diff/

Testing
-------

New test case checking that the parameter maxNotificationInterval can be configurable.

File Attachments
----------------

ZOOKEEPER-1814.patch
https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch

Thanks,

Daniel Peon

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by Daniel Peon <da...@ericsson.com>.


> On Dec. 11, 2013, 8:06 p.m., German Blanco wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 387
> > <https://reviews.apache.org/r/15753/diff/1/?file=389468#file389468line387>
> >
> >     Please remove spaces in this line.

I do not understand because I didn't modify the file QuorumPeer.java. Actually there is no modification information on the patch that I uploaded. Is it something that I'm missing or missunderstanding?


On Dec. 11, 2013, 8:06 p.m., Daniel Peon wrote:
> > In general review indentation (spaces instead of tabs), remove trailing spaces.
> > Also, there is no test case included. If the test was in a new file, check that you have done "svn add" before generating the patch.

I think something wrong happened with the patch generation. Although the .patch file does not contain some of the lines that you could find in the diff command, they are changes that I did before in the code.

I just generated a new patch from the LATEST version. Additionally I performed the "svn add" operation before generating the patch also.

Please review it if you can just and let's see if I did it right.


- Daniel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/#review30222
-----------------------------------------------------------


On Dec. 12, 2013, 8:30 a.m., Daniel Peon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15753/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2013, 8:30 a.m.)
> 
> 
> Review request for zookeeper, German Blanco and fpj.
> 
> 
> Bugs: ZOOKEEPER-1814
>     https://issues.apache.org/jira/browse/ZOOKEEPER-1814
> 
> 
> Repository: zookeeper
> 
> 
> Description
> -------
> 
> See Zookeeper-1814.
> 
> 
> Diffs
> -----
> 
>   ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1550368 
>   ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1550368 
>   ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1550368 
>   ./src/java/test/org/apache/zookeeper/test/FLEMaxIntervalNotificationTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/15753/diff/
> 
> 
> Testing
> -------
> 
> New test case checking that the parameter maxNotificationInterval can be configurable.
> 
> The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.
> 
> 
> File Attachments
> ----------------
> 
> ZOOKEEPER-1814.patch
>   https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch
> 
> 
> Thanks,
> 
> Daniel Peon
> 
>

Re: Review Request 15753: Reduction of waiting time during Fast Leader Election (ZOOKEEPER-1814)

Posted by German Blanco <ge...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15753/#review30222
-----------------------------------------------------------



./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java
<https://reviews.apache.org/r/15753/#comment57829>

    This line is commented, should the change be removed?



./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
<https://reviews.apache.org/r/15753/#comment57830>

    Please remove spaces in this line.



./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
<https://reviews.apache.org/r/15753/#comment57831>

    Please remove spaces.



./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
<https://reviews.apache.org/r/15753/#comment57832>

    This comment is empty, I guess it should be removed.



./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
<https://reviews.apache.org/r/15753/#comment57833>

    Please change tab to spaces, and also remove trailing spaces (marked with red color).


In general review indentation (spaces instead of tabs), remove trailing spaces.
Also, there is no test case included. If the test was in a new file, check that you have done "svn add" before generating the patch.

- German Blanco


On Dec. 11, 2013, 8:36 a.m., Daniel Peon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15753/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2013, 8:36 a.m.)
> 
> 
> Review request for zookeeper, German Blanco and fpj.
> 
> 
> Bugs: ZOOKEEPER-1814
>     https://issues.apache.org/jira/browse/ZOOKEEPER-1814
> 
> 
> Repository: zookeeper
> 
> 
> Description
> -------
> 
> See Zookeeper-1814.
> 
> 
> Diffs
> -----
> 
>   ./src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1541529 
>   ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1541529 
>   ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1541529 
>   ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 1541529 
>   ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1541529 
> 
> Diff: https://reviews.apache.org/r/15753/diff/
> 
> 
> Testing
> -------
> 
> New test case checking that the parameter maxNotificationInterval can be configurable.
> 
> The test case overwrites the receiveConnection method in QuorumCnxManager class in order to avoid any answer to Fast Leader Election. Thus, FLE is forced to retry increasing exponentially the notifications interval. This interval is measured and the test case stops when 3 consecutive measures are equal. The test case will be OK if the measure is the configured in the cfg file.
> 
> 
> File Attachments
> ----------------
> 
> ZOOKEEPER-1814.patch
>   https://reviews.apache.org/media/uploaded/files/2013/12/11/7e510364-6df3-486e-b338-3398960ba6b0__ZOOKEEPER-1814.patch
> 
> 
> Thanks,
> 
> Daniel Peon
> 
>