You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Rakesh R <ra...@huawei.com> on 2014/03/21 08:55:19 UTC
Review Request 19522: ZOOKEEPER-1263: Fix handling of min/max session
timeout value initialization
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19522/
-----------------------------------------------------------
Review request for zookeeper, michim and Raul Gutierrez Segales.
Bugs: ZOOKEEPER-1263
https://issues.apache.org/jira/browse/ZOOKEEPER-1263
Repository: zookeeper
Description
-------
Fix handling of min/max session timeout value initialization. Addresses foollowing cases:-
1) Server startup fails if configured only with the 'minSessionTimeout' and not 'maxSessionTimeout'
2) Log is showing -1 as min/max session timeout if there is no sessiontimeout value configured
Diffs
-----
./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1579879
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1579879
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1579879
./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1579879
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 1579879
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1579879
Diff: https://reviews.apache.org/r/19522/diff/
Testing
-------
Tests covering standalone & quorum cases.
Thanks,
Rakesh R
Re: Review Request 19522: ZOOKEEPER-1263: Fix handling of min/max session
timeout value initialization
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19522/#review38498
-----------------------------------------------------------
Ship it!
Ship It!
- Raul Gutierrez Segales
On March 25, 2014, 6:01 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19522/
> -----------------------------------------------------------
>
> (Updated March 25, 2014, 6:01 p.m.)
>
>
> Review request for zookeeper, fpj, michim, Patrick Hunt, and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1263
> https://issues.apache.org/jira/browse/ZOOKEEPER-1263
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Fix handling of min/max session timeout value initialization. Addresses foollowing cases:-
> 1) Server startup fails if configured only with the 'minSessionTimeout' and not 'maxSessionTimeout'
> 2) Log is showing -1 as min/max session timeout if there is no sessiontimeout value configured
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1581425
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1581425
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1581425
> ./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1581425
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 1581425
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1581425
>
> Diff: https://reviews.apache.org/r/19522/diff/
>
>
> Testing
> -------
>
> Tests covering standalone & quorum cases.
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19522: ZOOKEEPER-1263: Fix handling of min/max session
timeout value initialization
Posted by mi...@cs.stanford.edu.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19522/#review38530
-----------------------------------------------------------
Ship it!
Ship It!
- michim
On March 25, 2014, 6:01 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19522/
> -----------------------------------------------------------
>
> (Updated March 25, 2014, 6:01 p.m.)
>
>
> Review request for zookeeper, fpj, michim, Patrick Hunt, and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1263
> https://issues.apache.org/jira/browse/ZOOKEEPER-1263
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Fix handling of min/max session timeout value initialization. Addresses foollowing cases:-
> 1) Server startup fails if configured only with the 'minSessionTimeout' and not 'maxSessionTimeout'
> 2) Log is showing -1 as min/max session timeout if there is no sessiontimeout value configured
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1581425
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1581425
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1581425
> ./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1581425
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 1581425
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1581425
>
> Diff: https://reviews.apache.org/r/19522/diff/
>
>
> Testing
> -------
>
> Tests covering standalone & quorum cases.
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19522: ZOOKEEPER-1263: Fix handling of min/max session
timeout value initialization
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19522/#review38492
-----------------------------------------------------------
./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java
<https://reviews.apache.org/r/19522/#comment70699>
nit: spaces:
```
if (configs != null) {
```
./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java
<https://reviews.apache.org/r/19522/#comment70696>
nit: just String hostport, no need to make it all uppercase like a constant (sorry i missed this before).
- Raul Gutierrez Segales
On March 25, 2014, 6:01 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19522/
> -----------------------------------------------------------
>
> (Updated March 25, 2014, 6:01 p.m.)
>
>
> Review request for zookeeper, fpj, michim, Patrick Hunt, and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1263
> https://issues.apache.org/jira/browse/ZOOKEEPER-1263
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Fix handling of min/max session timeout value initialization. Addresses foollowing cases:-
> 1) Server startup fails if configured only with the 'minSessionTimeout' and not 'maxSessionTimeout'
> 2) Log is showing -1 as min/max session timeout if there is no sessiontimeout value configured
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1581425
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1581425
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1581425
> ./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1581425
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 1581425
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1581425
>
> Diff: https://reviews.apache.org/r/19522/diff/
>
>
> Testing
> -------
>
> Tests covering standalone & quorum cases.
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19522: ZOOKEEPER-1263: Fix handling of min/max session
timeout value initialization
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19522/
-----------------------------------------------------------
(Updated March 25, 2014, 6:01 p.m.)
Review request for zookeeper, fpj, michim, Patrick Hunt, and Raul Gutierrez Segales.
Changes
-------
Thanks Michi, Raul for the reviews and your time. I'm uploading the latest patch.
I've done the changes except the duplicate condition present in ZKServer, for that pls see my reply.
Bugs: ZOOKEEPER-1263
https://issues.apache.org/jira/browse/ZOOKEEPER-1263
Repository: zookeeper
Description
-------
Fix handling of min/max session timeout value initialization. Addresses foollowing cases:-
1) Server startup fails if configured only with the 'minSessionTimeout' and not 'maxSessionTimeout'
2) Log is showing -1 as min/max session timeout if there is no sessiontimeout value configured
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1581425
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1581425
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1581425
./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1581425
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 1581425
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1581425
Diff: https://reviews.apache.org/r/19522/diff/
Testing
-------
Tests covering standalone & quorum cases.
Thanks,
Rakesh R
Re: Review Request 19522: ZOOKEEPER-1263: Fix handling of min/max session
timeout value initialization
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19522/#review38330
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
<https://reviews.apache.org/r/19522/#comment70376>
nit: drop the ()s, doesn't add value since it isn't a compound expression.
./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
<https://reviews.apache.org/r/19522/#comment70377>
i think you want this.minSessionTimeout not min.
./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
<https://reviews.apache.org/r/19522/#comment70378>
same comments as for setMinSessionTimeout.
./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java
<https://reviews.apache.org/r/19522/#comment70386>
why not PortAssignment.unique()?
./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java
<https://reviews.apache.org/r/19522/#comment70385>
why not PortAssignment.unique()?
./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java
<https://reviews.apache.org/r/19522/#comment70384>
why do we need to create/get anything? sounds like asserting you that the negotiated timeout is what was expected should be enough no?
- Raul Gutierrez Segales
On March 21, 2014, 7:58 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19522/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 7:58 a.m.)
>
>
> Review request for zookeeper, fpj, michim, Patrick Hunt, and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1263
> https://issues.apache.org/jira/browse/ZOOKEEPER-1263
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Fix handling of min/max session timeout value initialization. Addresses foollowing cases:-
> 1) Server startup fails if configured only with the 'minSessionTimeout' and not 'maxSessionTimeout'
> 2) Log is showing -1 as min/max session timeout if there is no sessiontimeout value configured
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1579879
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1579879
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1579879
> ./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1579879
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 1579879
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1579879
>
> Diff: https://reviews.apache.org/r/19522/diff/
>
>
> Testing
> -------
>
> Tests covering standalone & quorum cases.
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19522: ZOOKEEPER-1263: Fix handling of min/max session
timeout value initialization
Posted by Rakesh R <ra...@huawei.com>.
> On March 23, 2014, 9:23 p.m., michim wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, line 283
> > <https://reviews.apache.org/r/19522/diff/2/?file=531098#file531098line283>
> >
> > Would it be possible to avoid duplicating this logic in QuorumPeerConfig.java and ZooKeeperServer.java?
> >
> > nit: wrap these lines at 80 characters.
Thanks Michi for the review. Yes true, but I've seen ZooKeeperServerBean has exposed setMinSessionTimeout() call, in that case ZooKeeperServer will use min/max session timeout conditions.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19522/#review38257
-----------------------------------------------------------
On March 21, 2014, 7:58 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19522/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 7:58 a.m.)
>
>
> Review request for zookeeper, fpj, michim, Patrick Hunt, and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1263
> https://issues.apache.org/jira/browse/ZOOKEEPER-1263
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Fix handling of min/max session timeout value initialization. Addresses foollowing cases:-
> 1) Server startup fails if configured only with the 'minSessionTimeout' and not 'maxSessionTimeout'
> 2) Log is showing -1 as min/max session timeout if there is no sessiontimeout value configured
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1579879
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1579879
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1579879
> ./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1579879
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 1579879
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1579879
>
> Diff: https://reviews.apache.org/r/19522/diff/
>
>
> Testing
> -------
>
> Tests covering standalone & quorum cases.
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19522: ZOOKEEPER-1263: Fix handling of min/max session
timeout value initialization
Posted by mi...@cs.stanford.edu.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19522/#review38257
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
<https://reviews.apache.org/r/19522/#comment70310>
Would it be possible to avoid duplicating this logic in QuorumPeerConfig.java and ZooKeeperServer.java?
nit: wrap these lines at 80 characters.
./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java
<https://reviews.apache.org/r/19522/#comment70311>
nit: wrap this line at 80 characters.
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java
<https://reviews.apache.org/r/19522/#comment70312>
nit: 80 characters
- michim
On March 21, 2014, 7:58 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19522/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 7:58 a.m.)
>
>
> Review request for zookeeper, fpj, michim, Patrick Hunt, and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1263
> https://issues.apache.org/jira/browse/ZOOKEEPER-1263
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Fix handling of min/max session timeout value initialization. Addresses foollowing cases:-
> 1) Server startup fails if configured only with the 'minSessionTimeout' and not 'maxSessionTimeout'
> 2) Log is showing -1 as min/max session timeout if there is no sessiontimeout value configured
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1579879
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1579879
> ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1579879
> ./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1579879
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 1579879
> ./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1579879
>
> Diff: https://reviews.apache.org/r/19522/diff/
>
>
> Testing
> -------
>
> Tests covering standalone & quorum cases.
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19522: ZOOKEEPER-1263: Fix handling of min/max session
timeout value initialization
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19522/
-----------------------------------------------------------
(Updated March 21, 2014, 7:58 a.m.)
Review request for zookeeper, fpj, michim, Patrick Hunt, and Raul Gutierrez Segales.
Bugs: ZOOKEEPER-1263
https://issues.apache.org/jira/browse/ZOOKEEPER-1263
Repository: zookeeper
Description
-------
Fix handling of min/max session timeout value initialization. Addresses foollowing cases:-
1) Server startup fails if configured only with the 'minSessionTimeout' and not 'maxSessionTimeout'
2) Log is showing -1 as min/max session timeout if there is no sessiontimeout value configured
Diffs
-----
./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1579879
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1579879
./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1579879
./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1579879
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 1579879
./src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 1579879
Diff: https://reviews.apache.org/r/19522/diff/
Testing
-------
Tests covering standalone & quorum cases.
Thanks,
Rakesh R