HBase
  1. HBase
  2. HBASE-5617

Provide coprocessor hooks in put flow while rollbackMemstore.

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 0.99.0
    • Component/s: Coprocessors
    • Labels:
      None

      Description

      With coprocessors hooks while put happens we have the provision to create new puts to other tables or regions. These puts can be done with writeToWal as false.
      In 0.94 and above the puts are first written to memstore and then to WAL. If any failure in the WAL append or sync the memstore is rollbacked.
      Now the problem is that if the put that happens in the main flow fails there is no way to rollback the
      puts that happened in the prePut.

      We can add coprocessor hooks to like pre/postRoolBackMemStore. Is any one hook enough here?

      1. HBASE-5617_2.patch
        20 kB
        ramkrishna.s.vasudevan
      2. HBASE-5617_1.patch
        19 kB
        ramkrishna.s.vasudevan

        Issue Links

          Activity

          Hide
          ramkrishna.s.vasudevan added a comment -

          Pls review the patch. This patch also adds additional hooks to the doMiniBatchPut.

          We found some gaps in the coprocessor hooks available and hence added the new hooks.
          For eg, the preBatchPut hook will help us to do any puts after the timestamp is updated in the kv and it will be called once with all the current puts that has acquired the locks.

          The postBatchPut hook is needed before the completeMemStoreInsert so that any new puts we do using the hooks can also adhere to the principles of mvcc.

          Similarly a final hook is also introduced so that any clean up operation can be done.
          Rollback memstore is also needed with a hook.
          Pls feel free to comment on the new hooks added.
          If only rollback memstore hook is needed then i will prepare a patch with that alone.
          This is just a version so that folks can get an idea on what has been added and where it has been added.

          Show
          ramkrishna.s.vasudevan added a comment - Pls review the patch. This patch also adds additional hooks to the doMiniBatchPut. We found some gaps in the coprocessor hooks available and hence added the new hooks. For eg, the preBatchPut hook will help us to do any puts after the timestamp is updated in the kv and it will be called once with all the current puts that has acquired the locks. The postBatchPut hook is needed before the completeMemStoreInsert so that any new puts we do using the hooks can also adhere to the principles of mvcc. Similarly a final hook is also introduced so that any clean up operation can be done. Rollback memstore is also needed with a hook. Pls feel free to comment on the new hooks added. If only rollback memstore hook is needed then i will prepare a patch with that alone. This is just a version so that folks can get an idea on what has been added and where it has been added.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12520114/HBASE-5617_1.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 6 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 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 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.replication.TestReplication
          org.apache.hadoop.hbase.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
          org.apache.hadoop.hbase.master.TestSplitLogManager

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1321//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1321//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1321//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/12520114/HBASE-5617_1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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 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 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.replication.TestReplication org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1321//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1321//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1321//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Haven't gone through the patch yet.
          The lines w.r.t. prePut() seem to be unrelated. It would be nice to remove them.

          Do we need two hooks for memstore rollback ?

          Show
          Ted Yu added a comment - Haven't gone through the patch yet. The lines w.r.t. prePut() seem to be unrelated. It would be nice to remove them. Do we need two hooks for memstore rollback ?
          Hide
          stack added a comment -

          No test Ram that new functionality works?

          Show
          stack added a comment - No test Ram that new functionality works?
          Hide
          Andrew Purtell added a comment -

          -1 on patch, API changes exceed the scope of this issue as defined.

          Show
          Andrew Purtell added a comment - -1 on patch, API changes exceed the scope of this issue as defined.
          Hide
          Anoop Sam John added a comment -

          Do we need two hooks for memstore rollback ?

          +1 for pre and post... As per the use case one can use pre or post hook...

          Show
          Anoop Sam John added a comment - Do we need two hooks for memstore rollback ? +1 for pre and post... As per the use case one can use pre or post hook...
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Andy
          Yes, this patch adds some more hooks other than the one specified in the JIRA title.
          My intention was to know if it is ok to add new hooks like the one in the patch.

          If it is fine i can raise another JIRA for the same and leave this JIRA with only hooks for rollbackOfMemStore.

          Show
          ramkrishna.s.vasudevan added a comment - @Andy Yes, this patch adds some more hooks other than the one specified in the JIRA title. My intention was to know if it is ok to add new hooks like the one in the patch. If it is fine i can raise another JIRA for the same and leave this JIRA with only hooks for rollbackOfMemStore.
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Stack
          I will be adding test cases for the rollback scenarios. I wanted to get the things clarified before giving a patch.

          Show
          ramkrishna.s.vasudevan added a comment - @Stack I will be adding test cases for the rollback scenarios. I wanted to get the things clarified before giving a patch.
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Andy
          This patch deals only with what is intended in the title of the JIRA.
          Can i raise another JIRA for adding other hooks?

          Show
          ramkrishna.s.vasudevan added a comment - @Andy This patch deals only with what is intended in the title of the JIRA. Can i raise another JIRA for adding other hooks?
          Hide
          Lars Hofhansl added a comment -

          In 0.94+ there are more places where we rollback the memstore.
          Check out HRegion.mutateRowsWithLocks in 0.94 and HRegion.processRowsWithLocks in 0.96.

          Show
          Lars Hofhansl added a comment - In 0.94+ there are more places where we rollback the memstore. Check out HRegion.mutateRowsWithLocks in 0.94 and HRegion.processRowsWithLocks in 0.96.
          Hide
          Ted Yu added a comment -

          rollbackMemstore() doesn't throw exception.
          I want to understand why we need two hooks for the rollback scenario.

          Show
          Ted Yu added a comment - rollbackMemstore() doesn't throw exception. I want to understand why we need two hooks for the rollback scenario.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12520451/HBASE-5617_2.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 6 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 these unit tests:
          org.apache.hadoop.hbase.regionserver.TestHRegion
          org.apache.hadoop.hbase.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1343//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1343//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1343//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/12520451/HBASE-5617_2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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 these unit tests: org.apache.hadoop.hbase.regionserver.TestHRegion org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1343//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1343//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1343//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Lars
          I will update in other places also.
          @Ted
          What do you think? Only one hook is enough? For our use case one hook is enough.
          From your previous comment i felt you suggested like its better we have two.

          Show
          ramkrishna.s.vasudevan added a comment - @Lars I will update in other places also. @Ted What do you think? Only one hook is enough? For our use case one hook is enough. From your previous comment i felt you suggested like its better we have two.
          Hide
          Ted Yu added a comment -

          I think one hook should be enough.

          Show
          Ted Yu added a comment - I think one hook should be enough.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Check out HRegion.mutateRowsWithLocks in 0.94 and HRegion.processRowsWithLocks in 0.96.

          These apis roll back the kvs directly. In my patch i thought of having putList.

          So can we now pass kvs?
          Or can we pass List<Mutation> and leave the user to do a instanceof check and proceed?

          Also is the name ok to have rollbackMemstore in it or only rollback? Kindly let me know your opinion on this.
          Also in trunk the processor.postProcess is done even if there is a failure. But in 0.94 its not.

          Show
          ramkrishna.s.vasudevan added a comment - Check out HRegion.mutateRowsWithLocks in 0.94 and HRegion.processRowsWithLocks in 0.96. These apis roll back the kvs directly. In my patch i thought of having putList. So can we now pass kvs? Or can we pass List<Mutation> and leave the user to do a instanceof check and proceed? Also is the name ok to have rollbackMemstore in it or only rollback? Kindly let me know your opinion on this. Also in trunk the processor.postProcess is done even if there is a failure. But in 0.94 its not.
          Hide
          Ted Yu added a comment -

          I think passing List<KeyValue> is general.

          rollbackMemstore would be a good name. See javadoc for Store.rollback().

          Show
          Ted Yu added a comment - I think passing List<KeyValue> is general. rollbackMemstore would be a good name. See javadoc for Store.rollback().
          Hide
          ramkrishna.s.vasudevan added a comment -

          I would like to revive this patch? Thougths...?

          Show
          ramkrishna.s.vasudevan added a comment - I would like to revive this patch? Thougths...?
          Hide
          ramkrishna.s.vasudevan added a comment -

          Am linking this to the Secondary index parent JIRA. If this hooks makes sense we can add the hooks.

          Show
          ramkrishna.s.vasudevan added a comment - Am linking this to the Secondary index parent JIRA. If this hooks makes sense we can add the hooks.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12520451/HBASE-5617_2.patch
          against trunk revision .

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8179//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/12520451/HBASE-5617_2.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 6 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8179//console This message is automatically generated.

            People

            • Assignee:
              ramkrishna.s.vasudevan
              Reporter:
              ramkrishna.s.vasudevan
            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:

                Development