ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1097

Quota is not correctly rehydrated on snapshot reload

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.3.3, 3.4.0
    • Fix Version/s: 3.3.4, 3.4.0
    • Component/s: server
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      traverseNode in DataTree will never actually traverse the limit nodes properly.

      1. ZOOKEEPER-1097-whitespace.patch
        9 kB
        Henry Robinson
      2. ZOOKEEPER-1097-whitespace.patch
        9 kB
        Henry Robinson
      3. ZOOKEEPER-1097-33.patch
        7 kB
        Camille Fournier
      4. ZOOKEEPER-1097.patch
        8 kB
        Camille Fournier
      5. ZOOKEEPER-1097.patch
        9 kB
        Camille Fournier
      6. ZOOKEEPER-1097
        12 kB
        Camille Fournier
      7. 1097.patch
        8 kB
        Henry Robinson

        Activity

        Hide
        Henry Robinson added a comment -

        Just committed this to 3.3. Thanks Camille!

        Show
        Henry Robinson added a comment - Just committed this to 3.3. Thanks Camille!
        Hide
        Camille Fournier added a comment -

        Yes still waiting to be checked in to 3.3.

        Show
        Camille Fournier added a comment - Yes still waiting to be checked in to 3.3.
        Hide
        Mahadev konar added a comment -

        Henry/Camille,
        The patch still needs to be committed to 3.3 branch right?

        Show
        Mahadev konar added a comment - Henry/Camille, The patch still needs to be committed to 3.3 branch right?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12483817/ZOOKEEPER-1097-33.patch
        against trunk revision 1139494.

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/352//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/12483817/ZOOKEEPER-1097-33.patch against trunk revision 1139494. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/352//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        Didn't touch data tree, just update the test. Hopefully all the whitespace is still good, I think I've rooted out all the places eclipse is putting tabs instead of spaces.

        Show
        Camille Fournier added a comment - Didn't touch data tree, just update the test. Hopefully all the whitespace is still good, I think I've rooted out all the places eclipse is putting tabs instead of spaces.
        Hide
        Camille Fournier added a comment -

        3.3.3 patch

        Show
        Camille Fournier added a comment - 3.3.3 patch
        Hide
        Camille Fournier added a comment -

        I'll take a look.

        Show
        Camille Fournier added a comment - I'll take a look.
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1224 (See https://builds.apache.org/job/ZooKeeper-trunk/1224/)
        ZOOKEEPER-1097. Quota is not correctly rehydrated on snapshot reload (camille via henryr)

        henry : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1139494
        Files :

        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ZooKeeperQuotaTest.java
        • /zookeeper/trunk/CHANGES.txt
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1224 (See https://builds.apache.org/job/ZooKeeper-trunk/1224/ ) ZOOKEEPER-1097 . Quota is not correctly rehydrated on snapshot reload (camille via henryr) henry : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1139494 Files : /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ZooKeeperQuotaTest.java /zookeeper/trunk/CHANGES.txt
        Hide
        Henry Robinson added a comment -

        committed to trunk; having trouble committing to branch-3.3 because the updated test seems to use an API that's not present in 3.3. Camille, any chance you can put together a patch that works on 3.3? Would be nice to have for 3.4.0. Alternatively, if there's a commit that I can cherry-pick from trunk that this patch depends on, let me know. (Sorry for the delay committing; git svn decided to blow up on me).

        Show
        Henry Robinson added a comment - committed to trunk; having trouble committing to branch-3.3 because the updated test seems to use an API that's not present in 3.3. Camille, any chance you can put together a patch that works on 3.3? Would be nice to have for 3.4.0. Alternatively, if there's a commit that I can cherry-pick from trunk that this patch depends on, let me know. (Sorry for the delay committing; git svn decided to blow up on me).
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12483331/ZOOKEEPER-1097-whitespace.patch
        against trunk revision 1138100.

        +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 (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/341//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/341//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/341//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/12483331/ZOOKEEPER-1097-whitespace.patch against trunk revision 1138100. +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 (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/341//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/341//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/341//console This message is automatically generated.
        Hide
        Henry Robinson added a comment -

        Camille - appreciate it, but I think I'm nearly done messing up the patch stupid git prefix... Curious about the test failures though.

        Show
        Henry Robinson added a comment - Camille - appreciate it, but I think I'm nearly done messing up the patch stupid git prefix... Curious about the test failures though.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12483331/ZOOKEEPER-1097-whitespace.patch
        against trunk revision 1138100.

        +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 (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://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/339//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/339//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/339//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/12483331/ZOOKEEPER-1097-whitespace.patch against trunk revision 1138100. +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 (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://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/339//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/339//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/339//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        I can deal with this if you want, Henry. Just let me know.

        Show
        Camille Fournier added a comment - I can deal with this if you want, Henry. Just let me know.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12483318/ZOOKEEPER-1097-whitespace.patch
        against trunk revision 1138100.

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/338//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/12483318/ZOOKEEPER-1097-whitespace.patch against trunk revision 1138100. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/338//console This message is automatically generated.
        Hide
        Henry Robinson added a comment -

        Sigh... messed up the last patch. Hopefully get this done today.

        Show
        Henry Robinson added a comment - Sigh... messed up the last patch. Hopefully get this done today.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12483029/1097.patch
        against trunk revision 1138100.

        +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 (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/335//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/335//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/335//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/12483029/1097.patch against trunk revision 1138100. +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 (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/335//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/335//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/335//console This message is automatically generated.
        Hide
        Henry Robinson added a comment -

        Kicking Jenkins, test failure seems unrelated.

        Show
        Henry Robinson added a comment - Kicking Jenkins, test failure seems unrelated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12483029/1097.patch
        against trunk revision 1136740.

        +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 (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://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/330//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/330//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/330//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/12483029/1097.patch against trunk revision 1136740. +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 (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://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/330//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/330//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/330//console This message is automatically generated.
        Hide
        Henry Robinson added a comment -

        You're right, for whatever reason JIRA put your most recent patch second in the list of attached files.

        Tabs vs. spaces: thankfully, it's not a matter of opinion (because let's face it, what could be more boring...): we have a consistent choice as per http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute. I've updated your patch with spaces for whitespace.

        We can ignore the inherited problems with those test lines, which are obviously just small nits, for now. Given that you were in the code, editing some of those lines, seemed like a good opportunity for cleanup. Next time!

        I'll commit this shortly. Thanks!

        Show
        Henry Robinson added a comment - You're right, for whatever reason JIRA put your most recent patch second in the list of attached files. Tabs vs. spaces: thankfully, it's not a matter of opinion (because let's face it, what could be more boring...): we have a consistent choice as per http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute . I've updated your patch with spaces for whitespace. We can ignore the inherited problems with those test lines, which are obviously just small nits, for now. Given that you were in the code, editing some of those lines, seemed like a good opportunity for cleanup. Next time! I'll commit this shortly. Thanks!
        Hide
        Henry Robinson added a comment -

        Updated patch, with tabs removed. Should be ready to go.

        Show
        Henry Robinson added a comment - Updated patch, with tabs removed. Should be ready to go.
        Hide
        Camille Fournier added a comment -

        The actual latest patch should have removed the eclipse project files. I suspect you looked at the one before that which I accidentally created.

        The assertTrue lines aren't part of my changes, except for a spelling correct. I just added the additional lines for actually verifying quota. Do you want me to rewrite the existing test to have assertEquals?

        If there are problems with whitespaces/tabs etc please feel free to fix them before committing, I really didn't intentionally do that and I have absolutely no opinions on formatting of anything at all. I make an effort to force my IDEs to put in spaces for tabs but sometimes I forget.

        Show
        Camille Fournier added a comment - The actual latest patch should have removed the eclipse project files. I suspect you looked at the one before that which I accidentally created. The assertTrue lines aren't part of my changes, except for a spelling correct. I just added the additional lines for actually verifying quota. Do you want me to rewrite the existing test to have assertEquals? If there are problems with whitespaces/tabs etc please feel free to fix them before committing, I really didn't intentionally do that and I have absolutely no opinions on formatting of anything at all. I make an effort to force my IDEs to put in spaces for tabs but sometimes I forget.
        Hide
        Henry Robinson added a comment -

        Thanks for updating the patch. Looks nearly good to go.

        • You've included a bunch of changes to eclipse project files. Can you remove them?
        • Javadoc looks good, thanks. In general, if you're making a method public for testing only, it's worth commenting on that. Guava has a nice @VisibleForTesting annotation that makes that explicit, but we don't depend on that right now. (It's weird that our tests are in different packages to the classes they test...)
        • FWIW, I really did mean assertEquals, for the lines where you have things like:

        Assert.assertTrue("count is set", qst.getCount() == 2);

        • Also turns out the messages in those asserts are inverted - they get shown when the assertion fails, so you'll see "Assertion failed: count is set" which is the precise opposite of what's gone wrong
        • ...aaaand while we're here, the indentation for the stopServer / startServer block seems to be tabs rather than spaces. Actually, there are a bunch of other places with tabs, I think. Would you mind changing them to be spaces consistently?

        Thanks for bearing with me.

        Show
        Henry Robinson added a comment - Thanks for updating the patch. Looks nearly good to go. You've included a bunch of changes to eclipse project files. Can you remove them? Javadoc looks good, thanks. In general, if you're making a method public for testing only, it's worth commenting on that. Guava has a nice @VisibleForTesting annotation that makes that explicit, but we don't depend on that right now. (It's weird that our tests are in different packages to the classes they test...) FWIW, I really did mean assertEquals, for the lines where you have things like: Assert.assertTrue("count is set", qst.getCount() == 2); Also turns out the messages in those asserts are inverted - they get shown when the assertion fails, so you'll see "Assertion failed: count is set" which is the precise opposite of what's gone wrong ...aaaand while we're here, the indentation for the stopServer / startServer block seems to be tabs rather than spaces. Actually, there are a bunch of other places with tabs, I think. Would you mind changing them to be spaces consistently? Thanks for bearing with me.
        Hide
        Hadoop QA added a comment -

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

        +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 (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/325//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/325//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/325//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/12482889/ZOOKEEPER-1097.patch against trunk revision 1136740. +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 (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/325//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/325//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/325//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        Will clean up small bits.

        Will rename isQuotaSet. It's really just exposed so this thing can be tested in some way. Do you still want javadoc?

        Yes, the start/stop/start/stop is necessary to get the load to be from snapshot, instead of txn log. Without that the test will pass with the bug still there.

        I think assertNotNull is best of all, if we're going to pick that particular nit. Or did you really want the actual value?

        Show
        Camille Fournier added a comment - Will clean up small bits. Will rename isQuotaSet. It's really just exposed so this thing can be tested in some way. Do you still want javadoc? Yes, the start/stop/start/stop is necessary to get the load to be from snapshot, instead of txn log. Without that the test will pass with the bug still there. I think assertNotNull is best of all, if we're going to pick that particular nit. Or did you really want the actual value?
        Hide
        Henry Robinson added a comment -

        Patch looks good to me, all I have are stylistic nits. Happy to commit after these are addressed.

        1. Trailing whitespace after:

        • if (thisStats.getCount() < updatedStat.getCount()) {
          + if (thisStats.getCount() > -1 && (thisStats.getCount() < updatedStat.getCount())) {

        2. You've reformatted one 'quota exceded' log message, but not the other. Would be good to be consistent.

        3. Should isQuotaSet be a public API? If so, it should have Javadoc. Either way, it's confusing for a method called is* to not return a boolean. Consider renaming it to, dunno, getMaxPrefixWithQuota ?

        4. The 'return null' else-clause of the if statement in isQuotaSet should be inside a { } pair, for consistency with the if clause.

        5. Just checking - the stop / start / stop / start at the end of testQuota needs to have two rounds of restarts to be a correct test?

        6. Generally better to use assertEquals rather than assertTrue when you are comparing values, it can help debuggers give better information about the test failure and makes it easier to write correct tests. (I know this is an inherited issue with this test).

        Show
        Henry Robinson added a comment - Patch looks good to me, all I have are stylistic nits. Happy to commit after these are addressed. 1. Trailing whitespace after: if (thisStats.getCount() < updatedStat.getCount()) { + if (thisStats.getCount() > -1 && (thisStats.getCount() < updatedStat.getCount())) { 2. You've reformatted one 'quota exceded' log message, but not the other. Would be good to be consistent. 3. Should isQuotaSet be a public API? If so, it should have Javadoc. Either way, it's confusing for a method called is* to not return a boolean. Consider renaming it to, dunno, getMaxPrefixWithQuota ? 4. The 'return null' else-clause of the if statement in isQuotaSet should be inside a { } pair, for consistency with the if clause. 5. Just checking - the stop / start / stop / start at the end of testQuota needs to have two rounds of restarts to be a correct test? 6. Generally better to use assertEquals rather than assertTrue when you are comparing values, it can help debuggers give better information about the test failure and makes it easier to write correct tests. (I know this is an inherited issue with this test).
        Hide
        Hadoop QA added a comment -

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

        +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 (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/322//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/322//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/322//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/12482839/ZOOKEEPER-1097.patch against trunk revision 1135515. +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 (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/322//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/322//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/322//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        Writing a test for this, hope to have the fix available by EOD

        Show
        Camille Fournier added a comment - Writing a test for this, hope to have the fix available by EOD

          People

          • Assignee:
            Camille Fournier
            Reporter:
            Camille Fournier
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development