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

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

+1 tests included. The patch appears to include 6 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/543//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/543//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/543//console

This message is automatically generated.

Could you please upload the patch to https://reviews.apache.org/r/new/ for review?

Hi Thomas,
Thanks for your interest; please see https://reviews.apache.org/r/1959/

-Eugene

Hi Patrick,

As with ZOOKEEPER-1181, I would like to lobby for this to be in 3.4.0. This fix addresses a significant functional bug in SASL authentication.

-Eugene

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1959/
-----------------------------------------------------------

(Updated 2011-09-22 18:33:42.805934)

Review request for zookeeper.

Changes
-------

add link to JIRA.

Summary
-------

There are 3 places where ClientCnxn should queue a AuthFailed event if client fails to authenticate. Without sending this event, clients may be stuck watching for a SaslAuthenticated event that will never come (since the client failed to authenticate).

This patch fixes SaslAuthFailTest.testBadSaslAuthNotifiesWatch() to test for the AuthFailed event : previously, the test was incorrectly not testing for this event.

It also removes the testBadSaslAuthNotifiesWatch() method from the SaslAuthTest class : this method belongs in SaslAuthFailTest, not SaslAuthTest. The former tests unsuccessful SASL authentication; the latter, successful SASL authentication.

This addresses bug ZOOKEEPER-1185.
https://issues.apache.org/jira/browse/ZOOKEEPER-1185

Diffs

src/java/main/org/apache/zookeeper/ClientCnxn.java db15348
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 43382c8
src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java 8de7c2a
src/java/test/org/apache/zookeeper/test/SaslAuthTest.java fd20346

Diff: https://reviews.apache.org/r/1959/diff

Testing
-------

All unit tests pass. Also tested with an HBase cluster with an hbase shell running as an unauthenticated Zookeeper client. As expected, hbase shell could not access cluster, but, as expected, did not hang.

Thanks,

Eugene

Eugene,
Just a minor nit, commented on the review board. Other than that it looks good. Ill go ahead and commit it to 3.4 and 3.5 as soon as its updated.

thanks!

Hi Mahadev, I didn't see your comment on https://reviews.apache.org/r/1959/ - did you not publish it perhaps?
-Eugene

patch that fixes the following statement in ClientCnxn.java from:

if ((p.ctx == null) || (clientCnxn.zooKeeperSaslClient == null) ||
                              (clientCnxn.zooKeeperSaslClient.getSaslState() == ZooKeeperSaslClient.SaslState.FAILED))

to

if ((clientCnxn == null) || (clientCnxn.zooKeeperSaslClient == null) ||
                              (clientCnxn.zooKeeperSaslClient.getSaslState() == ZooKeeperSaslClient.SaslState.FAILED))

Just committed this. Thanks Eugene. I ran tests manually to make sure my changes were fine.

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

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

+1 tests included. The patch appears to include 6 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/590//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/590//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/590//console

This message is automatically generated.

Excellent, thank you Mahadev.

Integrated in ZooKeeper-trunk #1316 (See https://builds.apache.org/job/ZooKeeper-trunk/1316/)
ZOOKEEPER-1185. Send AuthFailed event to client if SASL authentication fails. (Eugene Kuntz via mahadev)

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

• /zookeeper/trunk/CHANGES.txt
• /zookeeper/trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java
• /zookeeper/trunk/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java
• /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java
• /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthTest.java