[
https://issues.apache.org/jira/browse/ZOOKEEPER-1246?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13142159#comment-13142159
]
[email protected] commented on ZOOKEEPER-1246:
----------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2671/
-----------------------------------------------------------
Review request for zookeeper.
Summary
-------
This is _not_ a diff againt current trunk but against the trunk _before_ the
first version of the ZK-1246 patch got committed.
This addresses bug ZOOKEEPER-1246.
https://issues.apache.org/jira/browse/ZOOKEEPER-1246
Diffs
-----
CHANGES.txt 8ed2bc2
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 1a80d74
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java
PRE-CREATION
Diff: https://reviews.apache.org/r/2671/diff
Testing
-------
Thanks,
Thomas
> 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: Thomas Koch
> Priority: Blocker
> Fix For: 3.4.0, 3.5.0
>
> Attachments: ZOOKEEPER-1246.patch, ZOOKEEPER-1246.patch,
> 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
