[ https://issues.apache.org/jira/browse/ZOOKEEPER-1246?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13136184#comment-13136184 ]
Hadoop QA commented on ZOOKEEPER-1246: -------------------------------------- +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12500923/ZOOKEEPER-1246_trunk.patch against trunk revision 1189318. +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/687//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/687//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/687//console This message is automatically generated. > Dead code in PrepRequestProcessor catch Exception block > ------------------------------------------------------- > > Key: ZOOKEEPER-1246 > URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1246 > Project: ZooKeeper > Issue Type: Sub-task > Reporter: Thomas Koch > Assignee: Camille Fournier > Priority: Blocker > Fix For: 3.4.0, 3.5.0 > > Attachments: ZOOKEEPER-1246.patch, ZOOKEEPER-1246.patch, > ZOOKEEPER-1246_trunk.patch, ZOOKEEPER-1246_trunk.patch > > > This is a regression introduced by ZOOKEEPER-965 (multi transactions). The > catch(Exception e) block in PrepRequestProcessor.pRequest contains an if > block with condition request.getHdr() != null. This condition will always > evaluate to false since the changes in ZOOKEEPER-965. > This is caused by a change in sequence: Before ZK-965, the txnHeader was set > _before_ the deserialization of the request. Afterwards the deserialization > happens before request.setHdr is set. So the following RequestProcessors > won't see the request as a failed one but as a Read request, since it doesn't > have a hdr set. > Notes: > - it is very bad practice to catch Exception. The block should rather catch > IOException > - The check whether the TxnHeader is set in the request is used at several > places to see whether the request is a read or write request. It isn't > obvious for a newby, what it means whether a request has a hdr set or not. > - at the beginning of pRequest the hdr and txn of request are set to null. > However there is no chance that these fields could ever not be null at this > point. The code however suggests that this could be the case. There should > rather be an assertion that confirms that these fields are indeed null. The > practice of doing things "just in case", even if there is no chance that this > case could happen, is a very stinky code smell and means that the code isn't > understandable or trustworthy. > - The multi transaction switch case block in pRequest is very hard to read, > because it missuses the request.{hdr|txn} fields as local variables. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira