[ 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