Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.4.0, 3.5.0
    • Component/s: None
    • Labels:
      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.

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

        Activity

        Hide
        Patrick Hunt added a comment -

        This sounds serious, until we triage it let's mark it as a blocker for 3.4.0.

        Show
        Patrick Hunt added a comment - This sounds serious, until we triage it let's mark it as a blocker for 3.4.0.
        Hide
        Camille Fournier added a comment -

        Ok, after a bit of looking it looks like what we need to do is catch IOException and appropriately raise that as a marshalling error. I am going to see what I can do to get a test for this.

        Show
        Camille Fournier added a comment - Ok, after a bit of looking it looks like what we need to do is catch IOException and appropriately raise that as a marshalling error. I am going to see what I can do to get a test for this.
        Hide
        Camille Fournier added a comment -

        Formatting may be wack and I haven't gone over it with a fine tooth comb but I think this patch takes care of it.

        Show
        Camille Fournier added a comment - Formatting may be wack and I haven't gone over it with a fine tooth comb but I think this patch takes care of it.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12500752/ZOOKEEPER-1246.patch
        against trunk revision 1188523.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/674//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12500752/ZOOKEEPER-1246.patch against trunk revision 1188523. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/674//console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Updated patch for current trunk.

        Show
        Patrick Hunt added a comment - Updated patch for current trunk.
        Hide
        Patrick Hunt added a comment -

        FYI, the original patch passed on my box with branch 34. (although I have not actually had time to review the patch itself for correctness)

        Show
        Patrick Hunt added a comment - FYI, the original patch passed on my box with branch 34. (although I have not actually had time to review the patch itself for correctness)
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12500909/ZOOKEEPER-1246_trunk.patch
        against trunk revision 1189318.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/686//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/686//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/686//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12500909/ZOOKEEPER-1246_trunk.patch against trunk revision 1189318. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/686//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/686//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/686//console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Added license header to new file.

        Show
        Patrick Hunt added a comment - Added license header to new file.
        Hide
        Patrick Hunt added a comment -

        Damn, missed svn add on the new file, also added license header to the new file.

        Show
        Patrick Hunt added a comment - Damn, missed svn add on the new file, also added license header to the new file.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12500923/ZOOKEEPER-1246_trunk.patch
        against trunk revision 1189318.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/687//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/687//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/687//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12500923/ZOOKEEPER-1246_trunk.patch against trunk revision 1189318. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/687//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/687//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/687//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12500923/ZOOKEEPER-1246_trunk.patch
        against trunk revision 1189318.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/688//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/688//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/688//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12500923/ZOOKEEPER-1246_trunk.patch against trunk revision 1189318. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/688//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/688//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/688//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        Thanks for migrating this to trunk, Patrick!

        Show
        Camille Fournier added a comment - Thanks for migrating this to trunk, Patrick!
        Hide
        Camille Fournier added a comment -

        FWIW I reviewed Patrick's port to trunk and it looks like he caught everything. If someone else can review this for correctness/style/etc I'm happy to fix any issues on 3.4 and trunk.

        Show
        Camille Fournier added a comment - FWIW I reviewed Patrick's port to trunk and it looks like he caught everything. If someone else can review this for correctness/style/etc I'm happy to fix any issues on 3.4 and trunk.
        Hide
        Mahadev konar added a comment -

        Looks good to me. Camille you want to check this in?

        Show
        Mahadev konar added a comment - Looks good to me. Camille you want to check this in?
        Hide
        Camille Fournier added a comment -

        Will do.

        Show
        Camille Fournier added a comment - Will do.
        Hide
        Camille Fournier added a comment -

        Oh brilliant, yet another refactoring blew away the trunk patch here.

        Show
        Camille Fournier added a comment - Oh brilliant, yet another refactoring blew away the trunk patch here.
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1352 (See https://builds.apache.org/job/ZooKeeper-trunk/1352/)
        ZOOKEEPER-1246. Dead code in PrepRequestProcessor catch Exception block (camille)

        camille : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1196025
        Files :

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1352 (See https://builds.apache.org/job/ZooKeeper-trunk/1352/ ) ZOOKEEPER-1246 . Dead code in PrepRequestProcessor catch Exception block (camille) camille : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1196025 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java
        Hide
        Thomas Koch added a comment -

        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:

          if(Request.isQuorum()) {
            request.setHdr( ... )
          }
        
        Show
        Thomas Koch added a comment - 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: if (Request.isQuorum()) { request.setHdr( ... ) }
        Hide
        Thomas Koch added a comment -

        This patch

        • reverts the changes made to PrepRequestProcessor
        • fixes the issue instead by setting the Request hdr before entering the switch statement
        • removes the MySessionTracker implementation from the Test
        • changes the last catch block to catch IOException instead of Exception

        bonus:

        • The pRequest2Txn method lost the zxid parameter without being sad about it.
        • There's only one place remaining that calls ZooKeeperServer.getNextZxid()
        Show
        Thomas Koch added a comment - This patch reverts the changes made to PrepRequestProcessor fixes the issue instead by setting the Request hdr before entering the switch statement removes the MySessionTracker implementation from the Test changes the last catch block to catch IOException instead of Exception bonus: The pRequest2Txn method lost the zxid parameter without being sad about it. There's only one place remaining that calls ZooKeeperServer.getNextZxid()
        Hide
        Thomas Koch added a comment -
        • Forgot to mention: The original patch does not obey the 80 characters limit for newly introduced lines.

        I've renamed the test method now and added a comment to the test to relate it to this issue.

        Show
        Thomas Koch added a comment - Forgot to mention: The original patch does not obey the 80 characters limit for newly introduced lines. I've renamed the test method now and added a comment to the test to relate it to this issue.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12501955/ZOOKEEPER-1246.patch
        against trunk revision 1196025.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/762//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/762//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/762//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12501955/ZOOKEEPER-1246.patch against trunk revision 1196025. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/762//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/762//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/762//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12501957/ZOOKEEPER-1246.patch
        against trunk revision 1196025.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/764//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/764//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/764//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12501957/ZOOKEEPER-1246.patch against trunk revision 1196025. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/764//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/764//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/764//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
        Hide
        Camille Fournier added a comment -

        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.

        Show
        Camille Fournier added a comment - 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.
        Hide
        Patrick Hunt added a comment -

        Once something is committed we generally open a new jira to address further issues or insights people might have.

        Also, I pointed out on the list, if we want to catch such issues (eclipse warnings say) we need to include tests in the QABOT process - Thomas see my mail to the list on that. If you care about such things we need to enforce it through there as not everyone uses eclipse (same for findbugs, rat etc... that's already included by the bot.).

        Show
        Patrick Hunt added a comment - Once something is committed we generally open a new jira to address further issues or insights people might have. Also, I pointed out on the list, if we want to catch such issues (eclipse warnings say) we need to include tests in the QABOT process - Thomas see my mail to the list on that. If you care about such things we need to enforce it through there as not everyone uses eclipse (same for findbugs, rat etc... that's already included by the bot.).

          People

          • Assignee:
            Camille Fournier
            Reporter:
            Thomas Koch
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development