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

Posted to dev@zookeeper.apache.org by Bill Havanki <bh...@clouderagovt.com> on 2014/07/18 15:22:48 UTC

Review Request 23678: ZOOKEEPER-776 - minimum client session timeout

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

Review request for zookeeper.


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


Repository: zookeeper-git


Description
-------

Addition of a check that the client session timeout is at least at a minimum. The work here is based on the original patches from Gregory Haskins.

* In Java, the minimum is controlled by the zookeeper.minTimeout system property, with default of 100.
* In C, the minimum is fixed at 100.

Room for improvement (suggestions and assistance most welcome):

* There is no new unit test for the C client.
* The minimum in the C client is not configurable.
* The minimum enforcement is not yet documented.


Diffs
-----

  src/c/src/zookeeper.c 782ac52 
  src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 
  src/java/test/org/apache/zookeeper/ClientCnxnTest.java PRE-CREATION 

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


Testing
-------

Compiles, new unit test passes.


Thanks,

Bill Havanki

Re: Review Request 23678: ZOOKEEPER-776 - minimum client session timeout

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

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



src/c/src/zookeeper.c
<https://reviews.apache.org/r/23678/#comment84422>

    this constant could use a comment or a better name, i think. maybe MIN_ALLOWED_SESSION_TIMEOUT or at least a comment as to where in the protocol this is defined/mandated?



src/java/main/org/apache/zookeeper/ClientCnxn.java
<https://reviews.apache.org/r/23678/#comment84423>

    DEFAULT_MIN_TIMEOUT is misleading, maybe MIN_ALLOW_SESSION_TIMEOUT as well? or something along those lines


- Raul Gutierrez Segales


On July 18, 2014, 1:22 p.m., Bill Havanki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23678/
> -----------------------------------------------------------
> 
> (Updated July 18, 2014, 1:22 p.m.)
> 
> 
> Review request for zookeeper.
> 
> 
> Bugs: ZOOKEEPER-776
>     https://issues.apache.org/jira/browse/ZOOKEEPER-776
> 
> 
> Repository: zookeeper-git
> 
> 
> Description
> -------
> 
> Addition of a check that the client session timeout is at least at a minimum. The work here is based on the original patches from Gregory Haskins.
> 
> * In Java, the minimum is controlled by the zookeeper.minTimeout system property, with default of 100.
> * In C, the minimum is fixed at 100.
> 
> Room for improvement (suggestions and assistance most welcome):
> 
> * There is no new unit test for the C client.
> * The minimum in the C client is not configurable.
> * The minimum enforcement is not yet documented.
> 
> 
> Diffs
> -----
> 
>   src/c/src/zookeeper.c 782ac52 
>   src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 
>   src/java/test/org/apache/zookeeper/ClientCnxnTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23678/diff/
> 
> 
> Testing
> -------
> 
> Compiles, new unit test passes.
> 
> 
> Thanks,
> 
> Bill Havanki
> 
>