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

Posted to dev@curator.apache.org by joshng <gi...@git.apache.org> on 2018/12/04 21:42:17 UTC

[GitHub] curator pull request #284: CURATOR-489: CreateBuilderImpl assigns protectedI...

GitHub user joshng opened a pull request:

    https://github.com/apache/curator/pull/284

    CURATOR-489: CreateBuilderImpl assigns protectedId if doProtected is …

    This is a light change to correct a bug when `doProtected` is true: the existing code sets `CreateBuilderImpl.doProtected = true`, but assigns `protectedId = null`, resulting in a non-unique protection-token (`_c_null-`) being applied to the name created in zookeeper. Given that all replicas coordinating via zookeeper would use the identical token, this could conceivably lead to incorrect behavior when the protection logic is needed.
    
    While I believe this PR fixes the bug, I'd really suggest a more comprehensive change to eliminate the `CreateBuilderImpl.doProtected` field entirely, instead relying upon `protectedId != null` to convey this intention. However, being unfamiliar with the nuances of the existing implementation, I didn't want to perform that surgery today. If you think that change seems preferable, I'd be happy to do that instead.

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

    $ git pull https://github.com/joshng/curator CURATOR-489-set-protectedId

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

    https://github.com/apache/curator/pull/284.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 #284
    
----
commit 5c27ecc29c3503633462cd95971ceb6a91175404
Author: josh gruenberg <jo...@...>
Date:   2018-12-04T21:17:15Z

    CURATOR-489: CreateBuilderImpl assigns protectedId if doProtected is true

----


---

[GitHub] curator issue #284: CURATOR-489: CreateBuilderImpl assigns protectedId if do...

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

Github user Randgalt commented on the issue:

    https://github.com/apache/curator/pull/284
  
    Actually - I just opened a PR on your repo with a test


---

[GitHub] curator issue #284: CURATOR-489: CreateBuilderImpl assigns protectedId if do...

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

Github user joshng commented on the issue:

    https://github.com/apache/curator/pull/284
  
    Ok, that test is incorporated here now. 👍 


---

[GitHub] curator pull request #284: CURATOR-489: CreateBuilderImpl assigns protectedI...

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

Github user asfgit closed the pull request at:

    https://github.com/apache/curator/pull/284


---

[GitHub] curator issue #284: CURATOR-489: CreateBuilderImpl assigns protectedId if do...

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

Github user Randgalt commented on the issue:

    https://github.com/apache/curator/pull/284
  
    > suggest a more comprehensive change to eliminate the CreateBuilderImpl.doProtected field entirely
    
    Maybe a separate issue for that. 
    
    Do you mind writing a test for this please?


---