ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-767

Submitting Demo/Recipe Shared / Exclusive Lock Code

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 3.3.0
    • Fix Version/s: 3.5.0
    • Component/s: recipes
    • Labels:
      None
    • Release Note:
      New recipe code.

      Description

      Networked Insights would like to share-back some code for shared/exclusive locking that we are using in our labs.

      1. ZOOKEEPER-767.patch
        16 kB
        Sam Baskinger
      2. ZOOKEEPER-767.patch
        17 kB
        Patrick Hunt
      3. ZOOKEEPER-767.patch
        18 kB
        Sam Baskinger
      4. ZOOKEEPER-767.patch
        43 kB
        Sam Baskinger
      5. ZOOKEEPER-767.patch
        43 kB
        Sam Baskinger
      6. ZOOKEEPER-767.patch
        43 kB
        Tyler MacDonald

        Activity

        Hide
        Sam Baskinger added a comment -

        The package information has not been changed as I expect the maintainers would like to give this a suitable home and class name in the code base.

        Show
        Sam Baskinger added a comment - The package information has not been changed as I expect the maintainers would like to give this a suitable home and class name in the code base.
        Hide
        Sam Baskinger added a comment -

        Patch file for com.ni.zookeeper.utils.Lock.

        This patch should probably be renamed/repackaged after application.

        Show
        Sam Baskinger added a comment - Patch file for com.ni.zookeeper.utils.Lock. This patch should probably be renamed/repackaged after application.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12443763/Lock.java.patch
        against trunk revision 941473.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/82/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/82/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/82/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12443763/Lock.java.patch against trunk revision 941473. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/82/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/82/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/82/console This message is automatically generated.
        Hide
        Sam Baskinger added a comment -

        Submitting new patch with unit tests.

        Show
        Sam Baskinger added a comment - Submitting new patch with unit tests.
        Hide
        Sam Baskinger added a comment -

        Unit tests and new SharedExclusiveLock recipe implementation.

        Show
        Sam Baskinger added a comment - Unit tests and new SharedExclusiveLock recipe implementation.
        Hide
        Sam Baskinger added a comment -

        Patch file ZOOKEEPER-767.patch contains unit tests and an updated (fixed) implementation file.

        Show
        Sam Baskinger added a comment - Patch file ZOOKEEPER-767 .patch contains unit tests and an updated (fixed) implementation file.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12443889/ZOOKEEPER-767.patch
        against trunk revision 941521.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        -1 release audit. The applied patch generated 26 release audit warnings (more than the trunk's current 24 warnings).

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/85/testReport/
        Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/85/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/85/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/85/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12443889/ZOOKEEPER-767.patch against trunk revision 941521. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 release audit. The applied patch generated 26 release audit warnings (more than the trunk's current 24 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/85/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/85/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/85/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/85/console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Thanks Sam, release audit failure typically means that you are missing license headers in your new source files. Could you update the patch? (checkout the existing source files for example of what the license header should be)

        Show
        Patrick Hunt added a comment - Thanks Sam, release audit failure typically means that you are missing license headers in your new source files. Could you update the patch? (checkout the existing source files for example of what the license header should be)
        Hide
        Sam Baskinger added a comment -

        I was wondering about that! Thanks, Patrick. I've got a few moments right now to get that updated.

        Naive question, do I need to invalidate / reflag the issue as having a patch for the build to pick it up? Thank you!

        Sam Baskinger

        Show
        Sam Baskinger added a comment - I was wondering about that! Thanks, Patrick. I've got a few moments right now to get that updated. Naive question, do I need to invalidate / reflag the issue as having a patch for the build to pick it up? Thank you! Sam Baskinger
        Hide
        Sam Baskinger added a comment -

        Unit tests and recipe implementation of a SharedExclusiveLock. This new attachment contains copyright/license information for the test class.

        Show
        Sam Baskinger added a comment - Unit tests and recipe implementation of a SharedExclusiveLock. This new attachment contains copyright/license information for the test class.
        Hide
        Sam Baskinger added a comment -

        This patch is the same as the previous one of the same name but with the license block added to the top of the test class.

        Show
        Sam Baskinger added a comment - This patch is the same as the previous one of the same name but with the license block added to the top of the test class.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12443903/ZOOKEEPER-767.patch
        against trunk revision 941521.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/86/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/86/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/86/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12443903/ZOOKEEPER-767.patch against trunk revision 941521. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/86/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/86/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/86/console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Sam, yes, you have to cancel/submit for the system to pickup the new attachment.

        Also you should just upload the new patch with the same name. JIRA will handle this properly (and as a result you can see the history of the patch as changes are made).

        Show
        Patrick Hunt added a comment - Sam, yes, you have to cancel/submit for the system to pickup the new attachment. Also you should just upload the new patch with the same name. JIRA will handle this properly (and as a result you can see the history of the patch as changes are made).
        Hide
        Patrick Hunt added a comment -

        Hi Sam, putting one's own copyright notice as done in this latest patch is against Apache policy (sort of like how we don't allow @author tags in the javadoc). One problem is that then everyone who patches the file is tempted to add a copyright for their contributions, this gets ugly fast. We don't try to fully document the reality of the copyright ownership in the source however the commit log / scm will document it, though.

        Also what you've added for the license is not the normal Apache boilerplate. It does not include the reference to NOTICE for copyright info like it should, it should also be at the very top of the file.

        Please update your patch with the following file header. (should be at the top of each source file, see the other files in this directory for examples). Thanks!

        /**

        • Licensed to the Apache Software Foundation (ASF) under one
        • or more contributor license agreements. See the NOTICE file
        • distributed with this work for additional information
        • regarding copyright ownership. The ASF licenses this file
        • to you under the Apache License, Version 2.0 (the
        • "License"); you may not use this file except in compliance
        • with the License. You may obtain a copy of the License at
          *
        • http://www.apache.org/licenses/LICENSE-2.0
          *
        • Unless required by applicable law or agreed to in writing, software
        • distributed under the License is distributed on an "AS IS" BASIS,
        • WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        • See the License for the specific language governing permissions and
        • limitations under the License.
          */
        Show
        Patrick Hunt added a comment - Hi Sam, putting one's own copyright notice as done in this latest patch is against Apache policy (sort of like how we don't allow @author tags in the javadoc). One problem is that then everyone who patches the file is tempted to add a copyright for their contributions, this gets ugly fast. We don't try to fully document the reality of the copyright ownership in the source however the commit log / scm will document it, though. Also what you've added for the license is not the normal Apache boilerplate. It does not include the reference to NOTICE for copyright info like it should, it should also be at the very top of the file. Please update your patch with the following file header. (should be at the top of each source file, see the other files in this directory for examples). Thanks! /** Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at * http://www.apache.org/licenses/LICENSE-2.0 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */
        Hide
        Sam Baskinger added a comment -

        Implementation of a Shared/Exclusive lock per recipe page.

        Show
        Sam Baskinger added a comment - Implementation of a Shared/Exclusive lock per recipe page.
        Hide
        Sam Baskinger added a comment -

        Shared/Exclusive locking.

        Show
        Sam Baskinger added a comment - Shared/Exclusive locking.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12444731/ZOOKEEPER-767.patch
        against trunk revision 944515.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/93/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/93/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/93/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12444731/ZOOKEEPER-767.patch against trunk revision 944515. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/93/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/93/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/93/console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Fixed a few issues with the patch:

        1) recent changes to recipes test framework (junit 4.8) caused the tests to fail. I fixed this.
        2) pushed the licenses up to the top
        3) reformatted to code to be consistent with
        http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute
        4) a bit of cleanup in test setup/teardown and ensuring clients get closed in tests.

        Show
        Patrick Hunt added a comment - Fixed a few issues with the patch: 1) recent changes to recipes test framework (junit 4.8) caused the tests to fail. I fixed this. 2) pushed the licenses up to the top 3) reformatted to code to be consistent with http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute 4) a bit of cleanup in test setup/teardown and ensuring clients get closed in tests.
        Hide
        Patrick Hunt added a comment -

        Attached with the right file name this time.

        Show
        Patrick Hunt added a comment - Attached with the right file name this time.
        Hide
        Patrick Hunt added a comment -

        Mahadev can you review this one given you've worked on the other lock code quite a bit? Thanks.

        Show
        Patrick Hunt added a comment - Mahadev can you review this one given you've worked on the other lock code quite a bit? Thanks.
        Hide
        Sam Baskinger added a comment -

        Thank you for all this, Parick.

        Show
        Sam Baskinger added a comment - Thank you for all this, Parick.
        Hide
        Mahadev konar added a comment -

        pat, i will review it today sometime.

        Show
        Mahadev konar added a comment - pat, i will review it today sometime.
        Hide
        Benjamin Reed added a comment -

        -1 there are a couple of problems with the implementation:

        1) shouldn't you check to see if you already have a lock before you do the create? that will remove the code right after the create in the getXXXXLock() methods.

        2) if you already have an exclusive lock, shouldn't that also count as a shared lock?

        3) the error handling is a bit problematic. a connection loss exception or an interrupt can leave a process holding a lock without knowing it.

        4) when you go through the children, you may end up checking for the existence of every znode before you, which could be wasteful.

        i think it may be better to expand the current locking code to handle shared lock rather than add a new lock implementation. the current lock recipe implementation only does exclusive locks, but it is implemented in a way that makes it easy to support shared locks as well and it takes care of the above problems.

        Show
        Benjamin Reed added a comment - -1 there are a couple of problems with the implementation: 1) shouldn't you check to see if you already have a lock before you do the create? that will remove the code right after the create in the getXXXXLock() methods. 2) if you already have an exclusive lock, shouldn't that also count as a shared lock? 3) the error handling is a bit problematic. a connection loss exception or an interrupt can leave a process holding a lock without knowing it. 4) when you go through the children, you may end up checking for the existence of every znode before you, which could be wasteful. i think it may be better to expand the current locking code to handle shared lock rather than add a new lock implementation. the current lock recipe implementation only does exclusive locks, but it is implemented in a way that makes it easy to support shared locks as well and it takes care of the above problems.
        Hide
        Sam Baskinger added a comment -

        Thanks for the feedback Benjamin,

        Perhaps I misread the recipe or am missing the philosophy of ZK's atomicity. It wouldn't be the first time. To your points:

        We do the create to ensure that we, at some point, will hold a lock. I do want to do the create, ensuring my turn, and then wait until I'm at the front of the line (front being defined in the exclusive or shared way).

        There should be a unit test that ensure that this does indeed happen, semantically. Exclusive locks block all shared access, if I take your meaning correctly.

        I thought the API guaranteed that in the event of a connection loss the EPHEMERAL creation property would guarantee that when the session timed out the file would be removed and watchers would be signaled.

        All but those behind me in the line of locks. This could certainly be optimized and is something I thought about, but moved past to get the rough implementation in flight.

        If the above 4 points hold, then extending the other implementation may be better for the community. I hope you'll include the code, but if not, we're very happy with it and appreciate ZooKeeper! Keep up the fine work.

        What do you think? What did I miss?

        Sam Baskinger

        Show
        Sam Baskinger added a comment - Thanks for the feedback Benjamin, Perhaps I misread the recipe or am missing the philosophy of ZK's atomicity. It wouldn't be the first time. To your points: We do the create to ensure that we, at some point, will hold a lock. I do want to do the create, ensuring my turn, and then wait until I'm at the front of the line (front being defined in the exclusive or shared way). There should be a unit test that ensure that this does indeed happen, semantically. Exclusive locks block all shared access, if I take your meaning correctly. I thought the API guaranteed that in the event of a connection loss the EPHEMERAL creation property would guarantee that when the session timed out the file would be removed and watchers would be signaled. All but those behind me in the line of locks. This could certainly be optimized and is something I thought about, but moved past to get the rough implementation in flight. If the above 4 points hold, then extending the other implementation may be better for the community. I hope you'll include the code, but if not, we're very happy with it and appreciate ZooKeeper! Keep up the fine work. What do you think? What did I miss? Sam Baskinger
        Hide
        Sam Baskinger added a comment -

        Thanks for the feedback Benjamin,

        Replying by email removed snippets from your message. Same comments as above, but with quotes for context (and fewer smilies).

        Perhaps I misread the recipe or am missing the philosophy of ZK's atomicity. It wouldn't be the first time. To your points:

        > 1) shouldn't you check to see if you already have a lock before you do the create? that will remove the code right after the create in the getXXXXLock() methods.

        We do the create to ensure that we, at some point, will hold a lock. I do want to do the create, ensuring my turn, and then wait until I'm at the front of the line (front being defined in the exclusive or shared way).

        > 2) if you already have an exclusive lock, shouldn't that also count as a shared lock?

        There should be a unit test that ensure that this does indeed happen, semantically. Exclusive locks block all shared access, if I take your meaning correctly.

        > 3) the error handling is a bit problematic. a connection loss exception or an interrupt can leave a process holding a lock without knowing it.

        I thought the API guaranteed that in the event of a connection loss the EPHEMERAL creation property would guarantee that when the session timed out the file would be removed and watchers would be signaled.

        > 4) when you go through the children, you may end up checking for the existence of every znode before you, which could be wasteful.

        All but those behind me in the line of locks. This could certainly be optimized and is something I thought about, but moved past to get the rough implementation in flight.

        > i think it may be better to expand the current locking code to handle shared lock rather than add a new lock implementation. the current lock recipe
        > implementation only does exclusive locks, but it is implemented in a way that makes it easy to support shared locks as well and it takes care of the
        > above problems.

        If the above 4 points hold, then extending the other implementation may be better for the community. I hope you'll include the code, but if not, we're very happy with it and appreciate ZooKeeper! Keep up the fine work.

        What do you think? What did I miss?

        Sam Baskinger

        Show
        Sam Baskinger added a comment - Thanks for the feedback Benjamin, Replying by email removed snippets from your message. Same comments as above, but with quotes for context (and fewer smilies). Perhaps I misread the recipe or am missing the philosophy of ZK's atomicity. It wouldn't be the first time. To your points: > 1) shouldn't you check to see if you already have a lock before you do the create? that will remove the code right after the create in the getXXXXLock() methods. We do the create to ensure that we, at some point, will hold a lock. I do want to do the create, ensuring my turn, and then wait until I'm at the front of the line (front being defined in the exclusive or shared way). > 2) if you already have an exclusive lock, shouldn't that also count as a shared lock? There should be a unit test that ensure that this does indeed happen, semantically. Exclusive locks block all shared access, if I take your meaning correctly. > 3) the error handling is a bit problematic. a connection loss exception or an interrupt can leave a process holding a lock without knowing it. I thought the API guaranteed that in the event of a connection loss the EPHEMERAL creation property would guarantee that when the session timed out the file would be removed and watchers would be signaled. > 4) when you go through the children, you may end up checking for the existence of every znode before you, which could be wasteful. All but those behind me in the line of locks. This could certainly be optimized and is something I thought about, but moved past to get the rough implementation in flight. > i think it may be better to expand the current locking code to handle shared lock rather than add a new lock implementation. the current lock recipe > implementation only does exclusive locks, but it is implemented in a way that makes it easy to support shared locks as well and it takes care of the > above problems. If the above 4 points hold, then extending the other implementation may be better for the community. I hope you'll include the code, but if not, we're very happy with it and appreciate ZooKeeper! Keep up the fine work. What do you think? What did I miss? Sam Baskinger
        Hide
        Benjamin Reed added a comment -

        1) just to make sure we are talking about the same thing. this is the code i'm referring to:

        // Check that we don't already have a lock...
        if (currentExclusiveLock != null && !isExpired(currentExclusiveLock)) {
                   // We have the exclusive lock! Remove newly made lock file and just
                   // return.
                   zooKeeper.delete(writeLock, -1);
                   return currentExclusiveLock;
        }
        

        2) no, i'm talking about when you go to get the shared lock, you first check to see if you have a shared lock. shouldn't you check for both shared and exclusive?

        3) the problem is that connection loss and session expiration are different. with connection loss you will get an exception, but your session can recover and you can keep using it. for session expired you are right the EPHEMERAL will go away. in the connection loss scenario you have a situation where you may acquire a lock but not know it.

        with regard to the question of current lock implementation in the repository. i'm trying to understand the differences with that implementation and yours. both follow the same recipe right? if the current lock implementation implemented shared locks, would you have used that one? or is there something more fundamental?

        Show
        Benjamin Reed added a comment - 1) just to make sure we are talking about the same thing. this is the code i'm referring to: // Check that we don't already have a lock... if (currentExclusiveLock != null && !isExpired(currentExclusiveLock)) { // We have the exclusive lock! Remove newly made lock file and just // return. zooKeeper.delete(writeLock, -1); return currentExclusiveLock; } 2) no, i'm talking about when you go to get the shared lock, you first check to see if you have a shared lock. shouldn't you check for both shared and exclusive? 3) the problem is that connection loss and session expiration are different. with connection loss you will get an exception, but your session can recover and you can keep using it. for session expired you are right the EPHEMERAL will go away. in the connection loss scenario you have a situation where you may acquire a lock but not know it. with regard to the question of current lock implementation in the repository. i'm trying to understand the differences with that implementation and yours. both follow the same recipe right? if the current lock implementation implemented shared locks, would you have used that one? or is there something more fundamental?
        Hide
        Sam Baskinger added a comment -

        1) Moving lock-ownership checks before we perform a zookeeper action.
        2) Wrapping any code that may throw an exception after creating a lock file with a catch block that will delete the lock file and propagate the exception up the call stack.

        Show
        Sam Baskinger added a comment - 1) Moving lock-ownership checks before we perform a zookeeper action. 2) Wrapping any code that may throw an exception after creating a lock file with a catch block that will delete the lock file and propagate the exception up the call stack.
        Hide
        Sam Baskinger added a comment -

        Thanks of the code-snippet Benjamin. You're absolutely right. Fixed #1.

        Regarding #2, when getting a shared lock we ignore existing shared locks and only look for exclusive locks. Line 225 of the new patch has:

        if (child.startsWith(EXLOCK)) { ...
        

        If I'm not confusing the matter, while a single "exclusive lock" node represents a single exclusive lock, a series of contiguous "shared lock" nodes make up the total of a shared lock. I took some time to stare at the code in question and corresponding code in the getExclusiveLock() call and I think they are as we intended them.

        As for #3, wow, I fell asleep at the IDE for that one. Thank you. Any exception will result in a "roll back" of the lock file creation and the Exception is propagated up the stack.

        Now, the larger question of the existing lock implementation, the existing WriteLock.java doesn't appear to closely follow the recipe (I'm reading http://hadoop.apache.org/zookeeper/docs/current/recipes.html#sc_recipes_Locks ) . What would prevent us from using it is the lack of first scheduling a lock (creating the node) and then doing the blocking logic. We realize this is potentially more work, but there may be some very high reader contention and we need to ensure that a single writer process doesn't starve. There is the added benefit of being able to observe the finite list of readers that must complete before the writer can lock.

        Other than that, if the existing WriteLock had shared/exclusive coexisting and a block-until-timeout construct, we would probably prefer to spend our time integrating that code than crafting up our own. It may well be that the SharedExclusiveLock.java file has too many production concerns in it and doesn't suite the goal of a recipe file.

        Show
        Sam Baskinger added a comment - Thanks of the code-snippet Benjamin. You're absolutely right. Fixed #1. Regarding #2, when getting a shared lock we ignore existing shared locks and only look for exclusive locks. Line 225 of the new patch has: if (child.startsWith(EXLOCK)) { ... If I'm not confusing the matter, while a single "exclusive lock" node represents a single exclusive lock, a series of contiguous "shared lock" nodes make up the total of a shared lock. I took some time to stare at the code in question and corresponding code in the getExclusiveLock() call and I think they are as we intended them. As for #3, wow, I fell asleep at the IDE for that one. Thank you. Any exception will result in a "roll back" of the lock file creation and the Exception is propagated up the stack. Now, the larger question of the existing lock implementation, the existing WriteLock.java doesn't appear to closely follow the recipe (I'm reading http://hadoop.apache.org/zookeeper/docs/current/recipes.html#sc_recipes_Locks ) . What would prevent us from using it is the lack of first scheduling a lock (creating the node) and then doing the blocking logic. We realize this is potentially more work, but there may be some very high reader contention and we need to ensure that a single writer process doesn't starve. There is the added benefit of being able to observe the finite list of readers that must complete before the writer can lock. Other than that, if the existing WriteLock had shared/exclusive coexisting and a block-until-timeout construct, we would probably prefer to spend our time integrating that code than crafting up our own. It may well be that the SharedExclusiveLock.java file has too many production concerns in it and doesn't suite the goal of a recipe file.
        Hide
        Sam Baskinger added a comment -

        Fixed failure cases in the event of a thrown exception after requesting a lock. Optimization in returning earlier (and avoiding a needless create operation) when a lock file is already held by the caller. Also, some variable names were updated from the read/write to shared/exclusive.

        Show
        Sam Baskinger added a comment - Fixed failure cases in the event of a thrown exception after requesting a lock. Optimization in returning earlier (and avoiding a needless create operation) when a lock file is already held by the caller. Also, some variable names were updated from the read/write to shared/exclusive.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12447056/ZOOKEEPER-767.patch
        against trunk revision 953041.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/115/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/115/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/115/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12447056/ZOOKEEPER-767.patch against trunk revision 953041. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/115/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/115/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/115/console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        sam,
        sorry for the delay in review. Any chance you'd be providing a c implementation of shared locks as well?

        Show
        Mahadev konar added a comment - sam, sorry for the delay in review. Any chance you'd be providing a c implementation of shared locks as well?
        Hide
        Sam Baskinger added a comment -

        I could certainly put some effort into that. My time's a little constrained now and I don't move nearly as quickly as I do in Java, but I try to get something written up.

        Show
        Sam Baskinger added a comment - I could certainly put some effort into that. My time's a little constrained now and I don't move nearly as quickly as I do in Java, but I try to get something written up.
        Hide
        Sam Baskinger added a comment -

        Patch containing java and c implementations of recipe locking code.

        Show
        Sam Baskinger added a comment - Patch containing java and c implementations of recipe locking code.
        Hide
        Sam Baskinger added a comment -

        Canceling last patch.

        Show
        Sam Baskinger added a comment - Canceling last patch.
        Hide
        Sam Baskinger added a comment -

        New patch contains java and c recipe code.

        Show
        Sam Baskinger added a comment - New patch contains java and c recipe code.
        Hide
        Sam Baskinger added a comment -

        Patch for java and c implementations of locking in zookeeper.

        Show
        Sam Baskinger added a comment - Patch for java and c implementations of locking in zookeeper.
        Hide
        Sam Baskinger added a comment -

        Includes C code.

        Show
        Sam Baskinger added a comment - Includes C code.
        Hide
        Mahadev konar added a comment -

        really good to see this. Ill try and review the code as soon as possible.

        Show
        Mahadev konar added a comment - really good to see this. Ill try and review the code as soon as possible.
        Hide
        Sam Baskinger added a comment -

        Found bug in C code. Line 202 - 204 does no initialize index 1 of the array but re-initializes index 0. Fixing coming next...

        Show
        Sam Baskinger added a comment - Found bug in C code. Line 202 - 204 does no initialize index 1 of the array but re-initializes index 0. Fixing coming next...
        Hide
        Sam Baskinger added a comment -

        This patch corrects an error in zoo_selock.c in which the second element of a two element array is not initialized before use.

        Show
        Sam Baskinger added a comment - This patch corrects an error in zoo_selock.c in which the second element of a two element array is not initialized before use.
        Hide
        Sam Baskinger added a comment -

        Corrects error of not initializing the second element of an array on line 202-204 of zoo_selock.c.

        Show
        Sam Baskinger added a comment - Corrects error of not initializing the second element of an array on line 202-204 of zoo_selock.c.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12457928/ZOOKEEPER-767.patch
        against trunk revision 1033155.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 8 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/16//testReport/
        Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/16//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/16//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12457928/ZOOKEEPER-767.patch against trunk revision 1033155. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/16//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/16//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/16//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12457928/ZOOKEEPER-767.patch
        against trunk revision 1055924.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 8 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 9 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/97//testReport/
        Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/97//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/97//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12457928/ZOOKEEPER-767.patch against trunk revision 1055924. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 9 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/97//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/97//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/97//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12457928/ZOOKEEPER-767.patch
        against trunk revision 1072085.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 8 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/152//testReport/
        Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/152//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/152//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12457928/ZOOKEEPER-767.patch against trunk revision 1072085. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/152//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/152//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/152//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12457928/ZOOKEEPER-767.patch
        against trunk revision 1072085.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 8 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/171//testReport/
        Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/171//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/171//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12457928/ZOOKEEPER-767.patch against trunk revision 1072085. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/171//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/171//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/171//console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        Moving it out, since this isnt critical or a blocker.

        Show
        Mahadev konar added a comment - Moving it out, since this isnt critical or a blocker.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12457928/ZOOKEEPER-767.patch
        against trunk revision 1144087.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 8 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/383//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/383//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/383//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12457928/ZOOKEEPER-767.patch against trunk revision 1144087. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/383//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/383//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/383//console This message is automatically generated.
        Hide
        Tyler MacDonald added a comment -

        Hi,
        There was a bug in the exclusive lock code that guaranteed that only the first locker would obtain the lock (any future lockers would wait in line forever). This is because one of the loops was in the wrong spot, and we were not resetting our state before checking if there was another lock. (So the first time through, we see there is another lock, and we assume that forever.)

        I've updated the patch to fix this bug.

        Show
        Tyler MacDonald added a comment - Hi, There was a bug in the exclusive lock code that guaranteed that only the first locker would obtain the lock (any future lockers would wait in line forever). This is because one of the loops was in the wrong spot, and we were not resetting our state before checking if there was another lock. (So the first time through, we see there is another lock, and we assume that forever.) I've updated the patch to fix this bug.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12495263/ZOOKEEPER-767.patch
        against trunk revision 1172406.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 8 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/570//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/570//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/570//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12495263/ZOOKEEPER-767.patch against trunk revision 1172406. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/570//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/570//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/570//console This message is automatically generated.
        Hide
        Sam Baskinger added a comment -

        Hi Tyler,

        Thanks for looking at the code!

        If I may be critical, it appears you created the problem of not reseting lockId = -1L by moving the lock checks outside of the for-loop that iterates over all existing children in ascending order (no new children should appear that have an ID below the one we established in exclusiveLockId.

        Other than that, it appears that the patch you submitted looks good! I just want to understand if there was really a defect or if this is perhaps a more efficient way of implementing the same functionality.

        Thanks again!

        Sam

        Show
        Sam Baskinger added a comment - Hi Tyler, Thanks for looking at the code! If I may be critical, it appears you created the problem of not reseting lockId = -1L by moving the lock checks outside of the for-loop that iterates over all existing children in ascending order (no new children should appear that have an ID below the one we established in exclusiveLockId . Other than that, it appears that the patch you submitted looks good! I just want to understand if there was really a defect or if this is perhaps a more efficient way of implementing the same functionality. Thanks again! Sam
        Hide
        Flavio Junqueira added a comment -

        Is this discussion a better match for Curator? Mahadev konar, you have commented previously in this jira, any other insight?

        Show
        Flavio Junqueira added a comment - Is this discussion a better match for Curator? Mahadev konar , you have commented previously in this jira, any other insight?
        Hide
        Mahadev konar added a comment -

        Flavio,
        Agreed, I think its definitely a better match for Curator.

        Show
        Mahadev konar added a comment - Flavio, Agreed, I think its definitely a better match for Curator.
        Hide
        Flavio Junqueira added a comment -

        I'm marking this jira as "won't fix". If necessary, move the discussion to curator or reopen it if there is anything we are missing about this issue.

        Show
        Flavio Junqueira added a comment - I'm marking this jira as "won't fix". If necessary, move the discussion to curator or reopen it if there is anything we are missing about this issue.

          People

          • Assignee:
            Sam Baskinger
            Reporter:
            Sam Baskinger
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - Not Specified
              Not Specified
              Remaining:
              Remaining Estimate - Not Specified
              Not Specified
              Logged:
              Time Spent - 8h
              8h

                Development