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

+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.

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).

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?

+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.

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.

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.

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

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!

-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.

Kicking Jenkins, test failure seems unrelated.

+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.

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

-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.

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

-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.

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

+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.

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).

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

I'll take a look.

3.3.3 patch

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.

-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.

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

Yes still waiting to be checked in to 3.3.

Just committed this to 3.3. Thanks Camille!