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

Posted to dev@zookeeper.apache.org by anmolnar <gi...@git.apache.org> on 2018/10/01 14:48:10 UTC

[GitHub] zookeeper pull request #651: ZOOKEEPER-3113 EphemeralType.get() fails to ver...

GitHub user anmolnar opened a pull request:

    https://github.com/apache/zookeeper/pull/651

    ZOOKEEPER-3113 EphemeralType.get() fails to verify ephemeralOwner when currentElapsedTime() is small enough

    I refactored the unit test `testServerIds` to verify server id verification code explicitly instead of through `EphemeralType.get()` method. 
    
    Reasons:
    - The original test doesn't work on machines which booted recently, because the generated `Time.currentElapsedTime()` value is not high enough and it's possible to generate a valid ephemeralOwner even with high 0xff byte. Server ID cannot be verified reliably this way.
    - EphemeralType.get() is already covered in other unit tests,  
    - Unit tests should test the smallest piece of logic and call the method under testing directly.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/anmolnar/zookeeper ZOOKEEPER-3113

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/zookeeper/pull/651.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #651
    
----
commit 659fc9a4158fed6a6c9eaca8b6725bdbdb9435c0
Author: Andor Molnar <an...@...>
Date:   2018-10-01T14:42:02Z

    ZOOKEEPER-3113. Refactored unit test to validate server Id verification code explicitly

----


---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by anmolnar <gi...@git.apache.org>.

Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    retest this please


---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by asfgit <gi...@git.apache.org>.

Github user asfgit commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    
    Refer to this link for build results (access rights to CI server needed): 
    https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2447/



---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by asfgit <gi...@git.apache.org>.

Github user asfgit commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    
    Refer to this link for build results (access rights to CI server needed): 
    https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2406/



---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by anmolnar <gi...@git.apache.org>.

Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    @hanm do you still -1 this?


---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by anmolnar <gi...@git.apache.org>.

Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    @hanm do you accept this patch with my recent changes?


---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by anmolnar <gi...@git.apache.org>.

Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    retest this please


---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by asfgit <gi...@git.apache.org>.

Github user asfgit commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    
    Refer to this link for build results (access rights to CI server needed): 
    https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2423/



---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by asfgit <gi...@git.apache.org>.

Github user asfgit commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    
    Refer to this link for build results (access rights to CI server needed): 
    https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2334/



---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by asfgit <gi...@git.apache.org>.

Github user asfgit commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    
    Refer to this link for build results (access rights to CI server needed): 
    https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2382/



---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by anmolnar <gi...@git.apache.org>.

Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    @hanm No problem. I'll write some new tests to validate the edge cases of `EphemeralType.get()`.


---

[GitHub] zookeeper issue #651: ZOOKEEPER-3113 EphemeralType.get() fails to verify eph...

Posted by hanm <gi...@git.apache.org>.

Github user hanm commented on the issue:

    https://github.com/apache/zookeeper/pull/651
  
    one comment - it would be good for the updated test of EphemeralType.get to cover the code path of throwing an IllegalArgumentException. This was what I referred to in my original comment about reduced code coverage, because neither of the new tests actually tests the presence of this exception under certain conditions.


---