[
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
