[
https://issues.apache.org/jira/browse/ZOOKEEPER-1246?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13142392#comment-13142392
]
Camille Fournier commented on ZOOKEEPER-1246:
---------------------------------------------
Thomas, a bit of feedback. This is unnecessarily aggressive and annoying, and
coming after I smacked you down for not writing tests for your own bugfixes it
makes you look incredibly petty and insecure. It is perfectly fair of you to
point out that I added an eclipse warning (guilty as charged, but if you really
care about these you need to make the build fail when additional warnings are
added). And yes, the formatting is not perfect. But as to most of the rest of
your points, you can frankly go to hell if you think I'm going to tolerate
being condescended to in this manner. You had the opportunity to fix this bug
yourself when you reported it. Instead, you pranced off to work on your own
thing and left it to me to debug and provide a fix. Now that the fix is done
and somehow not to your liking, the best you could hope for here is to request
a fix for the warning and formatting errors, and otherwise submit a new patch
as a refactor.
I'm closing this back up, and you are welcome to open a new issue with
formatting fixes/refactors on it if you so choose. But it is certainly not a
critical bug any longer.
> 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
