[
https://issues.apache.org/jira/browse/ZOOKEEPER-1246?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Thomas Koch reopened ZOOKEEPER-1246:
------------------------------------
I'm sorry to reopen this issue after the fact, but if everybody would review
every patch, we would loose a lot of time.
minor issues:
- The patch adds trailing whitespace.
- The new test file does not use unix newlines.
- Indentation in line 507 misses a space.
- The test file contains auto generated TODO comments that actually aren't
TODOs.
- The added if statements are not consistent. One of them uses braces, the
others don't.
- The test file introduces a new Warning about unused imports.
- The patch does change the method signature but not the method javadoc comment.
- The new added test case does not indicate what it actually tests. It could
either
- have a more explanatory name
- describe the tested Bug in a comment
- or just have at least the jira issue mentioned in a comment
- The test comes with an implementation of SessionTracker to set up a full
blown ZooKeeperServer. That's not necessary. One can just use the
LearnerSessionHandler.
major issues:
- The added Test isn't a Unit Test. It instantiates several other unrelated
classes and even touches network and file system.
- The patch increases the complexity of the code and couples it even more to
the Jute model of deserializing a pre-instantiated object.
- The patch adds another parameter to the pRequest2Txn method and thus also
makes it harder to read.
- The patch does not address the issue that Exception is catched in line 585
instead of IOException.
There would have been a trivial fix instead. Just add the setHdr statement
conditionally before the switch statement inside the try:
{code:java}
if(Request.isQuorum()) {
request.setHdr( ... )
}
{code}
> 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
