Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.2.2, 4.3.0
    • Component/s: bookkeeper-client
    • Labels:
      None

      Description

      currently, bookkeeper client still write ledger metadata to metadata storage even the metadata is already closed or undergoing closing. which would cause lots of bad version metadata update encountering unrecoverable errors in ledger handle. e.g. NotEnoughtBookiesException.

      1. BOOKKEEPER-580.diff
        8 kB
        Sijie Guo
      2. BOOKKEEPER-580.diff
        9 kB
        Sijie Guo
      3. 0001-BOOKKEEPER-580-improve-close-logic.patch
        9 kB
        Ivan Kelly
      4. 0001-BOOKKEEPER-580-improve-close-logic.patch
        3 kB
        Ivan Kelly

        Activity

        Hide
        Sijie Guo added a comment -

        attach a patch for it

        Show
        Sijie Guo added a comment - attach a patch for it
        Hide
        Hadoop QA added a comment -

        Testing JIRA BOOKKEEPER-580

        Patch BOOKKEEPER-580.diff downloaded at Wed Feb 27 08:10:46 UTC 2013

        ----------------------------

        +1 PATCH_APPLIES
        +1 CLEAN
        +1 RAW_PATCH_ANALYSIS
        . +1 the patch does not introduce any @author tags
        . +1 the patch does not introduce any tabs
        . +1 the patch does not introduce any trailing spaces
        . +1 the patch does not introduce any line longer than 120
        . +1 the patch does adds/modifies 4 testcase(s)
        +1 RAT
        . +1 the patch does not seem to introduce new RAT warnings
        +1 JAVADOC
        . +1 the patch does not seem to introduce new Javadoc warnings
        +1 COMPILE
        . +1 HEAD compiles
        . +1 patch compiles
        . +1 the patch does not seem to introduce new javac warnings
        +1 FINDBUGS
        . +1 the patch does not seem to introduce new Findbugs warnings
        +1 TESTS
        . Tests run: 817
        +1 DISTRO
        . +1 distro tarball builds with the patch

        ----------------------------
        +1 Overall result, good!, no -1s

        The full output of the test-patch run is available at

        . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/282/

        Show
        Hadoop QA added a comment - Testing JIRA BOOKKEEPER-580 Patch BOOKKEEPER-580.diff downloaded at Wed Feb 27 08:10:46 UTC 2013 ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 120 . +1 the patch does adds/modifies 4 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 FINDBUGS . +1 the patch does not seem to introduce new Findbugs warnings +1 TESTS . Tests run: 817 +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- +1 Overall result, good!, no -1s The full output of the test-patch run is available at . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/282/
        Hide
        Rakesh R added a comment -

        bookkeeper client still write ledger metadata to metadata storage even the metadata is already closed or undergoing closing. which would cause lots of bad version metadata update encountering unrecoverable errors in ledger handle. e.g. NotEnoughtBookiesException

        Sijie Guo If you could attach the log details, it would be helpful. I'm interested to know how NotEnoughtBookiesException occuring due to close/closing. Our existing #close() call is idempotent in nature, isn't it.
        Am I missing anything?

        Show
        Rakesh R added a comment - bookkeeper client still write ledger metadata to metadata storage even the metadata is already closed or undergoing closing. which would cause lots of bad version metadata update encountering unrecoverable errors in ledger handle. e.g. NotEnoughtBookiesException Sijie Guo If you could attach the log details, it would be helpful. I'm interested to know how NotEnoughtBookiesException occuring due to close/closing. Our existing #close() call is idempotent in nature, isn't it. Am I missing anything?
        Hide
        Sijie Guo added a comment -

        Rakesh R I said bad version updates after encountering NotEnoughBookies, not NotEnoughBookies during closing.

        please read the logic: Lost of PendingAdds failed at same time => Lots of LedgerHandle#handleUnrecoverableErrors (in parallel) => Lots of LedgerHandle#closeInternal (in parallel) => Lots of metadata updates (in parallel) => metadata version conflictions.

        Show
        Sijie Guo added a comment - Rakesh R I said bad version updates after encountering NotEnoughBookies, not NotEnoughBookies during closing. please read the logic: Lost of PendingAdds failed at same time => Lots of LedgerHandle#handleUnrecoverableErrors (in parallel) => Lots of LedgerHandle#closeInternal (in parallel) => Lots of metadata updates (in parallel) => metadata version conflictions.
        Hide
        Rakesh R added a comment -

        Yeah Sijie Guo. got it.

        Thanks for the patch, just few thoughts:

        • Could you please remove following java comments from #asyncCloseInternal() api, as now introducing meta.isClosed() check.
          // Close operation is idempotent, so no need to check if we are
          // already closed
          
        • There is an unncessary import in LedgerHandle.java. Anyway we are touching the file, it would be good to remove the unnecessary import also
          import org.apache.bookkeeper.client.BKException.BKNotEnoughBookiesException;
        • In the patch, I've seen #isClosed() checks in two places: first before submitting, also another one inside the SafeRunnable thread. I agree, there is no functional issue. Do we need double check?, I feel we can have only one which is inside the SafeRunnable.
          +        synchronized(this) {
          +            if (metadata.isClosed()) {
          +                cb.closeComplete(BKException.Code.LedgerClosedException, this, ctx);
          +                return;
          +            }
          +        }
          
        Show
        Rakesh R added a comment - Yeah Sijie Guo . got it. Thanks for the patch, just few thoughts: Could you please remove following java comments from #asyncCloseInternal() api, as now introducing meta.isClosed() check. // Close operation is idempotent, so no need to check if we are // already closed There is an unncessary import in LedgerHandle.java. Anyway we are touching the file, it would be good to remove the unnecessary import also import org.apache.bookkeeper.client.BKException.BKNotEnoughBookiesException; In the patch, I've seen #isClosed() checks in two places: first before submitting, also another one inside the SafeRunnable thread. I agree, there is no functional issue. Do we need double check?, I feel we can have only one which is inside the SafeRunnable. + synchronized ( this ) { + if (metadata.isClosed()) { + cb.closeComplete(BKException.Code.LedgerClosedException, this , ctx); + return ; + } + }
        Hide
        Sijie Guo added a comment -

        Rakesh R

        > There is an unncessary import in LedgerHandle.java.

        I could not be noticed since I used vim rather than an IDE. I think we should integrate checkstyle rather than ask someone to remove unnecessary imports by comments.

        > Do we need double check?, I feel we can have only one which is inside the SafeRunnable.

        I don't want to let lots of runnables propagated during failures, it might cause memory issue during a high throughput case.

        Show
        Sijie Guo added a comment - Rakesh R > There is an unncessary import in LedgerHandle.java. I could not be noticed since I used vim rather than an IDE. I think we should integrate checkstyle rather than ask someone to remove unnecessary imports by comments. > Do we need double check?, I feel we can have only one which is inside the SafeRunnable. I don't want to let lots of runnables propagated during failures, it might cause memory issue during a high throughput case.
        Hide
        Sijie Guo added a comment -

        remove mismatch comments in #asyncCloseInternal, remove unnecessary imports.

        Show
        Sijie Guo added a comment - remove mismatch comments in #asyncCloseInternal, remove unnecessary imports.
        Hide
        Hadoop QA added a comment -

        Testing JIRA BOOKKEEPER-580

        Patch BOOKKEEPER-580.diff downloaded at Tue Mar 5 21:29:29 UTC 2013

        ----------------------------

        +1 PATCH_APPLIES
        +1 CLEAN
        +1 RAW_PATCH_ANALYSIS
        . +1 the patch does not introduce any @author tags
        . +1 the patch does not introduce any tabs
        . +1 the patch does not introduce any trailing spaces
        . +1 the patch does not introduce any line longer than 120
        . +1 the patch does adds/modifies 4 testcase(s)
        +1 RAT
        . +1 the patch does not seem to introduce new RAT warnings
        +1 JAVADOC
        . +1 the patch does not seem to introduce new Javadoc warnings
        +1 COMPILE
        . +1 HEAD compiles
        . +1 patch compiles
        . +1 the patch does not seem to introduce new javac warnings
        +1 FINDBUGS
        . +1 the patch does not seem to introduce new Findbugs warnings
        +1 TESTS
        . Tests run: 817
        +1 DISTRO
        . +1 distro tarball builds with the patch

        ----------------------------
        +1 Overall result, good!, no -1s

        The full output of the test-patch run is available at

        . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/284/

        Show
        Hadoop QA added a comment - Testing JIRA BOOKKEEPER-580 Patch BOOKKEEPER-580.diff downloaded at Tue Mar 5 21:29:29 UTC 2013 ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 120 . +1 the patch does adds/modifies 4 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 FINDBUGS . +1 the patch does not seem to introduce new Findbugs warnings +1 TESTS . Tests run: 817 +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- +1 Overall result, good!, no -1s The full output of the test-patch run is available at . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/284/
        Hide
        Flavio Junqueira added a comment -

        I think we should integrate checkstyle

        +1, sounds like a good idea to use checkstyle.

        Show
        Flavio Junqueira added a comment - I think we should integrate checkstyle +1, sounds like a good idea to use checkstyle.
        Hide
        Rakesh R added a comment -

        Its nice to bring checkstyle.

        I don't want to let lots of runnables propagated during failures, it might cause memory issue during a high throughput case.

        Fine, make sense to me.

        Thanks Sijie Guo for the fix. +1 for the latest patch.

        Show
        Rakesh R added a comment - Its nice to bring checkstyle. I don't want to let lots of runnables propagated during failures, it might cause memory issue during a high throughput case. Fine, make sense to me. Thanks Sijie Guo for the fix. +1 for the latest patch.
        Hide
        Flavio Junqueira added a comment -

        Hi Sijie, I agree with the goal of not executing close operations unnecessarily, but I wonder why not return OK rather than LedgerClosedException.

        I'm not sure why you have removed the call to close in BookieRecoveryTest.

        About changing the exception caught in the following:

        -        } catch (BKException.BKMetadataVersionException e) {
        +        } catch (BKException.BKLedgerClosedException e) {
        

        We can still get metadata version exceptions in the case of concurrent attempts to close a ledger with different clients (distinct ledger handlers). In fact, I thought this was the case with TestFencing, so I'm wondering why it is correct to replace the exception like that.

        One other suggestion is to add comments describing why we need to check if the ledger is closed in those two places. These checks are there mostly for performance reasons, and for correctness I believe we don't need them, so it would be good to have some reminders there for us.

        Show
        Flavio Junqueira added a comment - Hi Sijie, I agree with the goal of not executing close operations unnecessarily, but I wonder why not return OK rather than LedgerClosedException. I'm not sure why you have removed the call to close in BookieRecoveryTest. About changing the exception caught in the following: - } catch (BKException.BKMetadataVersionException e) { + } catch (BKException.BKLedgerClosedException e) { We can still get metadata version exceptions in the case of concurrent attempts to close a ledger with different clients (distinct ledger handlers). In fact, I thought this was the case with TestFencing, so I'm wondering why it is correct to replace the exception like that. One other suggestion is to add comments describing why we need to check if the ledger is closed in those two places. These checks are there mostly for performance reasons, and for correctness I believe we don't need them, so it would be good to have some reminders there for us.
        Hide
        Sijie Guo added a comment -

        > but I wonder why not return OK rather than LedgerClosedException.

        I just want to make the return code clearly to tell what happened.

        > I'm not sure why you have removed the call to close in BookieRecoveryTest.

        since the ledger handle is already closed. we don't need to close it again.

        > In fact, I thought this was the case with TestFencing, so I'm wondering why it is correct to replace the exception like that.

        since closeLedgers is closed before recovering, so the ledger handles are already closed. closing them again is supposed to return LedgerClosedException. we don't get metadata version exception anymore. It is based on the new behavior to change that exception.

        > One other suggestion is to add comments describing why we need to check if the ledger is closed in those two places.

        I could do it if the above comments make sense for you.

        Show
        Sijie Guo added a comment - > but I wonder why not return OK rather than LedgerClosedException. I just want to make the return code clearly to tell what happened. > I'm not sure why you have removed the call to close in BookieRecoveryTest. since the ledger handle is already closed. we don't need to close it again. > In fact, I thought this was the case with TestFencing, so I'm wondering why it is correct to replace the exception like that. since closeLedgers is closed before recovering, so the ledger handles are already closed. closing them again is supposed to return LedgerClosedException. we don't get metadata version exception anymore. It is based on the new behavior to change that exception. > One other suggestion is to add comments describing why we need to check if the ledger is closed in those two places. I could do it if the above comments make sense for you.
        Hide
        Ivan Kelly added a comment -

        Why are we throwing an exception if a ledger is closed already? Closing a ledger should be an idempotent operation, so closing an already closed ledger shouldn't violate any of the assumptions the client has about the ledger handle.

        Show
        Ivan Kelly added a comment - Why are we throwing an exception if a ledger is closed already? Closing a ledger should be an idempotent operation, so closing an already closed ledger shouldn't violate any of the assumptions the client has about the ledger handle.
        Hide
        Ivan Kelly added a comment -

        Rebased to trunk (just had to fiddle with some import changes). Sijie Guo Flavio Junqueira Could we agree on what we do for a double invocation of close()? My opinion is that if we try to close an already closed handle, it should proceed with ok, as it doesn't change any assumptions on the state of the ledger.

        I'd like to get this into 4.2.2

        Show
        Ivan Kelly added a comment - Rebased to trunk (just had to fiddle with some import changes). Sijie Guo Flavio Junqueira Could we agree on what we do for a double invocation of close()? My opinion is that if we try to close an already closed handle, it should proceed with ok, as it doesn't change any assumptions on the state of the ledger. I'd like to get this into 4.2.2
        Hide
        Sijie Guo added a comment -

        I would make a change to not let sync version not throw exception on LedgerClosedException, but still expose the LedgerClosedException to callback version, so the user who want to identify the reason, it knows what happened. Is it OK for you guys?

        Show
        Sijie Guo added a comment - I would make a change to not let sync version not throw exception on LedgerClosedException, but still expose the LedgerClosedException to callback version, so the user who want to identify the reason, it knows what happened. Is it OK for you guys?
        Hide
        Ivan Kelly added a comment - - edited

        We should only inform the user of the exception if the user can take useful action as a result, (i.e. do something different with regard to that ledger).

        The user should only do something different it's view of the ledger is different after the close call, to what it was before the close call, specifically that the LAC is different. I think that fencing prevents this, but I need to think about it some more. If I can find a case that this is not true, then definitely we should have an exception (or maybe a read to check the last add confirmed of the closed metadata matches what the local handle has).

        I think the async and sync behaviour should be consistent though. Feels wrong to have the same api return a different result depending on how you call it.

        Show
        Ivan Kelly added a comment - - edited We should only inform the user of the exception if the user can take useful action as a result, (i.e. do something different with regard to that ledger). The user should only do something different it's view of the ledger is different after the close call, to what it was before the close call, specifically that the LAC is different. I think that fencing prevents this, but I need to think about it some more. If I can find a case that this is not true, then definitely we should have an exception (or maybe a read to check the last add confirmed of the closed metadata matches what the local handle has). I think the async and sync behaviour should be consistent though. Feels wrong to have the same api return a different result depending on how you call it.
        Hide
        Ivan Kelly added a comment -

        I had another look at the patch. The check is only protecting against 2 local calls of close(), fencing doesn't even come into it. The fencing case is handled in LedgerMetadata#resolveConflict(). The only justification for changing this for 2+ local calls would be to flag an error to the user. I don't think this is strong enough of a justification for a compatibility break though.

        Show
        Ivan Kelly added a comment - I had another look at the patch. The check is only protecting against 2 local calls of close(), fencing doesn't even come into it. The fencing case is handled in LedgerMetadata#resolveConflict(). The only justification for changing this for 2+ local calls would be to flag an error to the user. I don't think this is strong enough of a justification for a compatibility break though.
        Hide
        Ivan Kelly added a comment -

        Sijie Guo, could we get a conclusion on this? This is the last JIRA without a clear solution for 4.2.2

        Show
        Ivan Kelly added a comment - Sijie Guo , could we get a conclusion on this? This is the last JIRA without a clear solution for 4.2.2
        Hide
        Sijie Guo added a comment -

        Ivan Kelly it would be better to have a place to tell the client what happened to a ledger handle. the only thing I could image is through the rc code in call back. for sync version, we keep it not throw exception when a ledger is already closed.

        but I don't feel strong about this part. if you think we don't need to tell the client about ledger already closed, I am OK with that.

        Show
        Sijie Guo added a comment - Ivan Kelly it would be better to have a place to tell the client what happened to a ledger handle. the only thing I could image is through the rc code in call back. for sync version, we keep it not throw exception when a ledger is already closed. but I don't feel strong about this part. if you think we don't need to tell the client about ledger already closed, I am OK with that.
        Hide
        Ivan Kelly added a comment -

        I've changed the patch to complete with OK if the metadata is already closed. Its better to be backwards compatible in this case, and consistent between the async and sync versions. I did have to update one test, TestFencing, to not expect an exception any more, when another client has closed the ledger. This is fine though, as the LAC will be the same in the opening client and the closing client. No assumption has changed for the user. They will be notified of the fencing by the failure of the #addEntry call. If there is an incompatibility in the metadata (client has a lower LAC to what was closed with), LedgerMetadata#isConflictWith will catch it on the reread.

        Show
        Ivan Kelly added a comment - I've changed the patch to complete with OK if the metadata is already closed. Its better to be backwards compatible in this case, and consistent between the async and sync versions. I did have to update one test, TestFencing, to not expect an exception any more, when another client has closed the ledger. This is fine though, as the LAC will be the same in the opening client and the closing client. No assumption has changed for the user. They will be notified of the fencing by the failure of the #addEntry call. If there is an incompatibility in the metadata (client has a lower LAC to what was closed with), LedgerMetadata#isConflictWith will catch it on the reread.
        Hide
        Hadoop QA added a comment -

        Testing JIRA BOOKKEEPER-580

        Patch 0001-BOOKKEEPER-580-improve-close-logic.patch downloaded at Thu Aug 22 10:41:17 UTC 2013

        ----------------------------

        +1 PATCH_APPLIES
        +1 CLEAN
        +1 RAW_PATCH_ANALYSIS
        . +1 the patch does not introduce any @author tags
        . +1 the patch does not introduce any tabs
        . +1 the patch does not introduce any trailing spaces
        . +1 the patch does not introduce any line longer than 120
        . +1 the patch does adds/modifies 1 testcase(s)
        +1 RAT
        . +1 the patch does not seem to introduce new RAT warnings
        +1 JAVADOC
        . +1 the patch does not seem to introduce new Javadoc warnings
        +1 COMPILE
        . +1 HEAD compiles
        . +1 patch compiles
        . +1 the patch does not seem to introduce new javac warnings
        +1 FINDBUGS
        . +1 the patch does not seem to introduce new Findbugs warnings
        -1 TESTS
        . Tests run: 885
        . Tests failed: 7
        . Tests errors: 9

        . The patch failed the following testcases:

        . testSyncUnsubscribeWithInvalidSubscriberId[1](org.apache.hedwig.server.integration.TestHedwigHubRegular)
        . testAsyncUnsubscribeWithInvalidSubscriberId[1](org.apache.hedwig.server.integration.TestHedwigHubRegular)
        . testSyncHubSubscribeWithInvalidSubscriberId[1](org.apache.hedwig.server.integration.TestHedwigHubRegular)
        . testAsyncHubSubscribeWithInvalidSubscriberId[1](org.apache.hedwig.server.integration.TestHedwigHubRegular)
        . testSyncHubUnsubscribeWithInvalidSubscriberId[1](org.apache.hedwig.server.integration.TestHedwigHubRegular)
        . testAsyncHubUnsubscribeWithInvalidSubscriberId[1](org.apache.hedwig.server.integration.TestHedwigHubRegular)
        . testPublishWithBookKeeperError[1](org.apache.hedwig.server.integration.TestHedwigHubRegular)

        +1 DISTRO
        . +1 distro tarball builds with the patch

        ----------------------------
        -1 Overall result, please check the reported -1(s)

        The full output of the test-patch run is available at

        . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/470/

        Show
        Hadoop QA added a comment - Testing JIRA BOOKKEEPER-580 Patch 0001-BOOKKEEPER-580-improve-close-logic.patch downloaded at Thu Aug 22 10:41:17 UTC 2013 ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 120 . +1 the patch does adds/modifies 1 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 FINDBUGS . +1 the patch does not seem to introduce new Findbugs warnings -1 TESTS . Tests run: 885 . Tests failed: 7 . Tests errors: 9 . The patch failed the following testcases: . testSyncUnsubscribeWithInvalidSubscriberId [1] (org.apache.hedwig.server.integration.TestHedwigHubRegular) . testAsyncUnsubscribeWithInvalidSubscriberId [1] (org.apache.hedwig.server.integration.TestHedwigHubRegular) . testSyncHubSubscribeWithInvalidSubscriberId [1] (org.apache.hedwig.server.integration.TestHedwigHubRegular) . testAsyncHubSubscribeWithInvalidSubscriberId [1] (org.apache.hedwig.server.integration.TestHedwigHubRegular) . testSyncHubUnsubscribeWithInvalidSubscriberId [1] (org.apache.hedwig.server.integration.TestHedwigHubRegular) . testAsyncHubUnsubscribeWithInvalidSubscriberId [1] (org.apache.hedwig.server.integration.TestHedwigHubRegular) . testPublishWithBookKeeperError [1] (org.apache.hedwig.server.integration.TestHedwigHubRegular) +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/470/
        Hide
        Ivan Kelly added a comment -

        Rekicking build, as the failures seem to be something unrelated. Hedwig losing connection to ZK. Could be an tcp port conflict.

        Show
        Ivan Kelly added a comment - Rekicking build, as the failures seem to be something unrelated. Hedwig losing connection to ZK. Could be an tcp port conflict.
        Hide
        Hadoop QA added a comment -

        Testing JIRA BOOKKEEPER-580

        Patch 0001-BOOKKEEPER-580-improve-close-logic.patch downloaded at Thu Aug 22 12:10:06 UTC 2013

        ----------------------------

        +1 PATCH_APPLIES
        +1 CLEAN
        +1 RAW_PATCH_ANALYSIS
        . +1 the patch does not introduce any @author tags
        . +1 the patch does not introduce any tabs
        . +1 the patch does not introduce any trailing spaces
        . +1 the patch does not introduce any line longer than 120
        . +1 the patch does adds/modifies 1 testcase(s)
        +1 RAT
        . +1 the patch does not seem to introduce new RAT warnings
        +1 JAVADOC
        . +1 the patch does not seem to introduce new Javadoc warnings
        +1 COMPILE
        . +1 HEAD compiles
        . +1 patch compiles
        . +1 the patch does not seem to introduce new javac warnings
        +1 FINDBUGS
        . +1 the patch does not seem to introduce new Findbugs warnings
        +1 TESTS
        . Tests run: 877
        +1 DISTRO
        . +1 distro tarball builds with the patch

        ----------------------------
        +1 Overall result, good!, no -1s

        The full output of the test-patch run is available at

        . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/471/

        Show
        Hadoop QA added a comment - Testing JIRA BOOKKEEPER-580 Patch 0001-BOOKKEEPER-580-improve-close-logic.patch downloaded at Thu Aug 22 12:10:06 UTC 2013 ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 120 . +1 the patch does adds/modifies 1 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 FINDBUGS . +1 the patch does not seem to introduce new Findbugs warnings +1 TESTS . Tests run: 877 +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- +1 Overall result, good!, no -1s The full output of the test-patch run is available at . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/471/
        Hide
        Sijie Guo added a comment -

        +1 for the patch. thanks Ivan Kelly

        Show
        Sijie Guo added a comment - +1 for the patch. thanks Ivan Kelly
        Hide
        Ivan Kelly added a comment -

        Committed r1516929 to trunk.

        Show
        Ivan Kelly added a comment - Committed r1516929 to trunk.
        Hide
        Ivan Kelly added a comment -

        Committed revision 1516938 to branch-4.2

        Show
        Ivan Kelly added a comment - Committed revision 1516938 to branch-4.2
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in bookkeeper-trunk #337 (See https://builds.apache.org/job/bookkeeper-trunk/337/)
        BOOKKEEPER-580: improve close logic (sijie & ivank via ivank) (ivank: rev 1516929)

        • /zookeeper/bookkeeper/trunk/CHANGES.txt
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java
        Show
        Hudson added a comment - SUCCESS: Integrated in bookkeeper-trunk #337 (See https://builds.apache.org/job/bookkeeper-trunk/337/ ) BOOKKEEPER-580 : improve close logic (sijie & ivank via ivank) (ivank: rev 1516929) /zookeeper/bookkeeper/trunk/CHANGES.txt /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java

          People

          • Assignee:
            Ivan Kelly
            Reporter:
            Sijie Guo
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development