HBase
  1. HBase
  2. HBASE-10844

Coprocessor failure during batchmutation leaves the memstore datastructs in an inconsistent state

    Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: regionserver
    • Labels:
      None

      Description

      Observed this in the testing with Phoenix. The test in Phoenix - MutableIndexFailureIT deliberately fails the batchmutation call via the installed coprocessor. But the update is not rolled back. That leaves the memstore inconsistent. In particular, I observed that getFlushableSize is updated before the coprocessor was called but the update is not rolled back. When the region is being closed at some later point, the assert introduced in HBASE-10514 in the HRegion.doClose() causes the RegionServer to shutdown abnormally.

      1. 10844-1-0.98.txt
        1 kB
        Devaraj Das
      2. 10844-1.txt
        1 kB
        Devaraj Das

        Activity

        Hide
        Devaraj Das added a comment -

        Straightforward fix.

        Show
        Devaraj Das added a comment - Straightforward fix.
        Hide
        Devaraj Das added a comment -

        I should write a simple unit test. Will do so soon.

        Show
        Devaraj Das added a comment - I should write a simple unit test. Will do so soon.
        Hide
        Andrew Purtell added a comment -

        +1

        Show
        Andrew Purtell added a comment - +1
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12637029/10844-1.txt
        against trunk revision .
        ATTACHMENT ID: 12637029

        +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 appears to have generated 6 warning messages.

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

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

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

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

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

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//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/12637029/10844-1.txt against trunk revision . ATTACHMENT ID: 12637029 +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 appears to have generated 6 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9105//console This message is automatically generated.
        Hide
        Anoop Sam John added a comment -

        +1

        Show
        Anoop Sam John added a comment - +1
        Hide
        Anoop Sam John added a comment -

        Need a 94 patch also.

        Show
        Anoop Sam John added a comment - Need a 94 patch also.
        Hide
        ramkrishna.s.vasudevan added a comment -

        +1 on patch.

        Show
        ramkrishna.s.vasudevan added a comment - +1 on patch.
        Hide
        Anoop Sam John added a comment -

        Devaraj Das I guess we can commit this now.

        Show
        Anoop Sam John added a comment - Devaraj Das I guess we can commit this now.
        Hide
        Devaraj Das added a comment -

        Anoop Sam John, thanks for reminding. I'll commit this one today/tomorrow.

        Show
        Devaraj Das added a comment - Anoop Sam John , thanks for reminding. I'll commit this one today/tomorrow.
        Hide
        Lars Hofhansl added a comment -

        I do not think this is right.
        The point of no return is when the WAL is synced. If the region server dies after the sync the edit are replayed and hence the change will be visible. Hence the memstore should not be rolled back after the WAL edit is synced.

        postBatchMutate is "post" the batch mutate, so it cannot undo the mutation, it happened already.

        -1 on this change.

        Show
        Lars Hofhansl added a comment - I do not think this is right. The point of no return is when the WAL is synced. If the region server dies after the sync the edit are replayed and hence the change will be visible. Hence the memstore should not be rolled back after the WAL edit is synced. postBatchMutate is "post" the batch mutate, so it cannot undo the mutation, it happened already. -1 on this change.
        Hide
        Devaraj Das added a comment -

        Thanks Lars Hofhansl for catching this. Am busy since the last few days with some other stuff - will find some time to think about this shortly.

        Show
        Devaraj Das added a comment - Thanks Lars Hofhansl for catching this. Am busy since the last few days with some other stuff - will find some time to think about this shortly.
        Hide
        Lars Hofhansl added a comment -

        We could move the post hook before the syncOrDefer call... But it would not be a "post" hook anymore.
        Or we could explore why the action in question cannot be done in the pre hook.

        Show
        Lars Hofhansl added a comment - We could move the post hook before the syncOrDefer call... But it would not be a "post" hook anymore. Or we could explore why the action in question cannot be done in the pre hook.
        Hide
        Anoop Sam John added a comment -

        No Lars we cannot move before sync. This needs to be a post. Thanks for the catch. I have missed totally that we called sync b4 this cp call

        Show
        Anoop Sam John added a comment - No Lars we cannot move before sync. This needs to be a post. Thanks for the catch. I have missed totally that we called sync b4 this cp call
        Hide
        ramkrishna.s.vasudevan added a comment -

        Yes, remember now that we need cannot revert the memstore as it is synced up.
        sorry for the +1.
        If we set WriteToWal as false then rolling back of the memstore even after post call would make sense, right?

        Show
        ramkrishna.s.vasudevan added a comment - Yes, remember now that we need cannot revert the memstore as it is synced up. sorry for the +1. If we set WriteToWal as false then rolling back of the memstore even after post call would make sense, right?
        Hide
        Lars Hofhansl added a comment -

        If writeToWal is false for all mutations in this batch then the point of no return is the flushing of the memstore.

        Luckily the memstore flusher waits for all priot MVCC transactions to finish. And since in doMiniBatchMutation we call the post hook before mvcc.completeMemstoreInsert(w) we're safe from concurrent flushes.

        Hence in that case it would be OK in theory to fail post hook and then roll back the memstore.

        Show
        Lars Hofhansl added a comment - If writeToWal is false for all mutations in this batch then the point of no return is the flushing of the memstore. Luckily the memstore flusher waits for all priot MVCC transactions to finish. And since in doMiniBatchMutation we call the post hook before mvcc.completeMemstoreInsert(w) we're safe from concurrent flushes. Hence in that case it would be OK in theory to fail post hook and then roll back the memstore.
        Hide
        Anoop Sam John added a comment -

        IMO it will be good to be consistent with the sync wal case. ie even if WriteToWal is false, do not rollback with failures in post hooks. post means the mutation is done.

        Show
        Anoop Sam John added a comment - IMO it will be good to be consistent with the sync wal case. ie even if WriteToWal is false, do not rollback with failures in post hooks. post means the mutation is done.
        Hide
        Lars Hofhansl added a comment -

        Oh, I totally agree. I was just answering Ram's hypothetical question
        A post hook is a post hook, it should not undo anything.

        Show
        Lars Hofhansl added a comment - Oh, I totally agree. I was just answering Ram's hypothetical question A post hook is a post hook, it should not undo anything.
        Hide
        Lars Hofhansl added a comment -

        Should we close this as "invalid"?

        Show
        Lars Hofhansl added a comment - Should we close this as "invalid"?
        Hide
        Devaraj Das added a comment -

        Yes, Lars Hofhansl, I am resolving it as won't-fix.

        Show
        Devaraj Das added a comment - Yes, Lars Hofhansl , I am resolving it as won't-fix.
        Hide
        Devaraj Das added a comment -

        Thinking about it (and in offline discussions with Jeffrey Zhong and Enis Soztutar), it seems we should remove the 'assert' from the code that causes the RegionServer to abort. The reason being that coprocessors could fail. The assert was introduced in HBASE-10514.

                 // close each store in parallel
                 for (final Store store : stores.values()) {
        +          assert abort? true: store.getFlushableSize() == 0;
        

        Thoughts?

        Show
        Devaraj Das added a comment - Thinking about it (and in offline discussions with Jeffrey Zhong and Enis Soztutar ), it seems we should remove the 'assert' from the code that causes the RegionServer to abort. The reason being that coprocessors could fail. The assert was introduced in HBASE-10514 . // close each store in parallel for (final Store store : stores.values()) { + assert abort? true: store.getFlushableSize() == 0; Thoughts?
        Hide
        Devaraj Das added a comment -

        Patch for 0.98.

        Show
        Devaraj Das added a comment - Patch for 0.98.
        Hide
        Andrew Purtell added a comment -

        So with this patch we'd remove the assert and replace it with a warning that memstore datastructures have been only partially updated?

        --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        @@ -1109,7 +1109,13 @@ public class HRegion implements HeapSize { // , Writable{
         
                 // close each store in parallel
                 for (final Store store : stores.values()) {
        -          assert abort? true: store.getFlushableSize() == 0;
        +          if (store.getFlushableSize() != 0) {
        +            LOG.warn("store.getFlushableSize for " + store + " is not zero! It's " 
        +                + store.getFlushableSize() + ". Maybe a coprocessor "
        +                + "operation failed and "
        +                + "left the memstore datastructures in a partially updated state. "
        +                + "Current memstoreSize " + this.getMemstoreSize().get());
        +          }
                   completionService
                       .submit(new Callable<Pair<byte[], Collection<StoreFile>>>() {
                         @Override
        

        Shouldn't we be aborting in that case anyway? Or replace the assert with an abort()?

        Show
        Andrew Purtell added a comment - So with this patch we'd remove the assert and replace it with a warning that memstore datastructures have been only partially updated? --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -1109,7 +1109,13 @@ public class HRegion implements HeapSize { // , Writable{ // close each store in parallel for ( final Store store : stores.values()) { - assert abort? true : store.getFlushableSize() == 0; + if (store.getFlushableSize() != 0) { + LOG.warn( "store.getFlushableSize for " + store + " is not zero! It's " + + store.getFlushableSize() + ". Maybe a coprocessor " + + "operation failed and " + + "left the memstore datastructures in a partially updated state. " + + "Current memstoreSize " + this .getMemstoreSize().get()); + } completionService .submit( new Callable<Pair< byte [], Collection<StoreFile>>>() { @Override Shouldn't we be aborting in that case anyway? Or replace the assert with an abort()?

          People

          • Assignee:
            Devaraj Das
            Reporter:
            Devaraj Das
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:

              Development