Uploaded image for project: 'ZooKeeper'
  1. ZooKeeper
  2. ZOOKEEPER-1198 Refactorings and Cleanups
  3. ZOOKEEPER-1246

Dead code in PrepRequestProcessor catch Exception block

    XMLWordPrintableJSON

Details

    • Sub-task
    • Status: Closed
    • Blocker
    • Resolution: Fixed
    • None
    • 3.4.0, 3.5.0
    • None
    • None

    Description

      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.

      Attachments

        1. ZOOKEEPER-1246.patch
          12 kB
          Camille Fournier
        2. ZOOKEEPER-1246.patch
          13 kB
          Patrick D. Hunt
        3. ZOOKEEPER-1246.patch
          16 kB
          Thomas Koch
        4. ZOOKEEPER-1246.patch
          19 kB
          Thomas Koch
        5. ZOOKEEPER-1246_trunk.patch
          7 kB
          Patrick D. Hunt
        6. ZOOKEEPER-1246_trunk.patch
          12 kB
          Patrick D. Hunt

        Activity

          People

            fournc Camille Fournier
            thkoch Thomas Koch
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: