HBase
  1. HBase
  2. HBASE-5542

Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.95.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      mutateRowsWithLocks() does atomic mutations on multiple rows.
      processRow() does atomic read-modify-writes on a single row.

      It will be useful to generalize both and have a
      processRowsWithLocks() that does atomic read-modify-writes on multiple rows.

      This also helps reduce some redundancy in the codes.

      1. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.1.patch
        26 kB
        Phabricator
      2. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.10.patch
        66 kB
        Phabricator
      3. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.11.patch
        62 kB
        Phabricator
      4. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.12.patch
        62 kB
        Phabricator
      5. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.13.patch
        62 kB
        Phabricator
      6. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.14.patch
        62 kB
        Phabricator
      7. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.15.patch
        62 kB
        Phabricator
      8. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.2.patch
        27 kB
        Phabricator
      9. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.3.patch
        28 kB
        Phabricator
      10. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.4.patch
        58 kB
        Phabricator
      11. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.5.patch
        58 kB
        Phabricator
      12. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.6.patch
        58 kB
        Phabricator
      13. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.7.patch
        62 kB
        Phabricator
      14. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.8.patch
        63 kB
        Phabricator
      15. ASF.LICENSE.NOT.GRANTED--HBASE-5542.D2217.9.patch
        63 kB
        Phabricator
      16. HBASE-5542.2.txt
        68 kB
        Scott Chen
      17. HBASE-5542.3.txt
        66 kB
        Scott Chen
      18. HBASE-5542.4.txt
        66 kB
        Scott Chen
      19. HBASE-5542.4.txt
        66 kB
        Scott Chen
      20. HBASE-5542.4.txt
        66 kB
        Scott Chen
      21. HBASE-5542.txt
        68 kB
        Scott Chen

        Issue Links

          Activity

          Hide
          stack added a comment -

          Marking closed.

          Show
          stack added a comment - Marking closed.
          Hide
          Phabricator added a comment -

          sc has closed the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          To: tedyu, lhofhansl, JIRA, sc

          Show
          Phabricator added a comment - sc has closed the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". REVISION DETAIL https://reviews.facebook.net/D2217 To: tedyu, lhofhansl, JIRA, sc
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #155 (See https://builds.apache.org/job/HBase-TRUNK-security/155/)
          HBASE-5542 Addendum - accidentally checked in .orig file (Revision 1307562)

          Result = SUCCESS
          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java.orig
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #155 (See https://builds.apache.org/job/HBase-TRUNK-security/155/ ) HBASE-5542 Addendum - accidentally checked in .orig file (Revision 1307562) Result = SUCCESS larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java.orig
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2699 (See https://builds.apache.org/job/HBase-TRUNK/2699/)
          HBASE-5542 Addendum - accidentally checked in .orig file (Revision 1307562)

          Result = FAILURE
          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java.orig
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2699 (See https://builds.apache.org/job/HBase-TRUNK/2699/ ) HBASE-5542 Addendum - accidentally checked in .orig file (Revision 1307562) Result = FAILURE larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java.orig
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #146 (See https://builds.apache.org/job/HBase-TRUNK-security/146/)
          HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) part 2 (Revision 1303920)
          HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) (Revision 1303915)

          Result = SUCCESS
          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java.orig
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #146 (See https://builds.apache.org/job/HBase-TRUNK-security/146/ ) HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) part 2 (Revision 1303920) HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) (Revision 1303915) Result = SUCCESS larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java.orig /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2692 (See https://builds.apache.org/job/HBase-TRUNK/2692/)
          HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) part 2 (Revision 1303920)
          HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) (Revision 1303915)

          Result = SUCCESS
          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java.orig
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2692 (See https://builds.apache.org/job/HBase-TRUNK/2692/ ) HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) part 2 (Revision 1303920) HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) (Revision 1303915) Result = SUCCESS larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java.orig /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
          Hide
          Lars Hofhansl added a comment -

          Thanks Scott (for patch and verification )!

          Show
          Lars Hofhansl added a comment - Thanks Scott (for patch and verification )!
          Hide
          Scott Chen added a comment -

          @Lars: Synced and run TestAtomicOperation and TestRowProcessorEndpoint. It works fine. Thanks!

          Show
          Scott Chen added a comment - @Lars: Synced and run TestAtomicOperation and TestRowProcessorEndpoint. It works fine. Thanks!
          Hide
          Scott Chen added a comment -

          Thanks for committing this

          Show
          Scott Chen added a comment - Thanks for committing this
          Hide
          Scott Chen added a comment -

          @Lars: No problem. Let me do that right away.

          Show
          Scott Chen added a comment - @Lars: No problem. Let me do that right away.
          Hide
          Lars Hofhansl added a comment -

          Committed to trunk.
          Forgot new and deleted files in first commit.

          Scott, could you please sync trunk and double check and I fixed it all correctly with my 2nd commit?

          Show
          Lars Hofhansl added a comment - Committed to trunk. Forgot new and deleted files in first commit. Scott, could you please sync trunk and double check and I fixed it all correctly with my 2nd commit?
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #145 (See https://builds.apache.org/job/HBase-TRUNK-security/145/)
          HBASE-5542 revert due to broken trunk build (Revision 1303517)
          HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) (Revision 1303490)

          Result = SUCCESS
          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #145 (See https://builds.apache.org/job/HBase-TRUNK-security/145/ ) HBASE-5542 revert due to broken trunk build (Revision 1303517) HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) (Revision 1303490) Result = SUCCESS tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Lars Hofhansl added a comment -

          Looks good. Not for 0.94, so we can commit tomorrow.

          Show
          Lars Hofhansl added a comment - Looks good. Not for 0.94, so we can commit tomorrow.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12519383/HBASE-5542.4.txt
          against trunk revision .

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

          +1 tests included. The patch appears to include 4 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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1251//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1251//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1251//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/12519383/HBASE-5542.4.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1251//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1251//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1251//console This message is automatically generated.
          Hide
          Scott Chen added a comment -

          Attach v4 for the third time to pass Hadoop QA.

          Show
          Scott Chen added a comment - Attach v4 for the third time to pass Hadoop QA.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12519311/HBASE-5542.4.txt
          against trunk revision .

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

          +1 tests included. The patch appears to include 4 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 2 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.client.TestFromClientSide

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1243//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1243//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1243//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/12519311/HBASE-5542.4.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 2 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.client.TestFromClientSide Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1243//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1243//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1243//console This message is automatically generated.
          Hide
          Scott Chen added a comment -

          Attach one more time to gain more confidence from hadoop QA.

          Show
          Scott Chen added a comment - Attach one more time to gain more confidence from hadoop QA.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12519292/HBASE-5542.4.txt
          against trunk revision .

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

          +1 tests included. The patch appears to include 4 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 2 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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1239//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1239//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1239//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/12519292/HBASE-5542.4.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 2 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 Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1239//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1239//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1239//console This message is automatically generated.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2691 (See https://builds.apache.org/job/HBase-TRUNK/2691/)
          HBASE-5542 revert due to broken trunk build (Revision 1303517)

          Result = SUCCESS
          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2691 (See https://builds.apache.org/job/HBase-TRUNK/2691/ ) HBASE-5542 revert due to broken trunk build (Revision 1303517) Result = SUCCESS tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Scott Chen added a comment -

          Fixed the unit test.

          Show
          Scott Chen added a comment - Fixed the unit test.
          Hide
          Scott Chen added a comment -

          @Ted: Thanks. I will keep working on this. Sorry for the trouble.

          Show
          Scott Chen added a comment - @Ted: Thanks. I will keep working on this. Sorry for the trouble.
          Hide
          Ted Yu added a comment -

          Patch reverted to give Scott more time to investigate.

          Show
          Ted Yu added a comment - Patch reverted to give Scott more time to investigate.
          Hide
          Scott Chen added a comment -

          I ran the unit test many times and they passed.
          I will keep investigating this. But I don't think it is a big issue now.

          Show
          Scott Chen added a comment - I ran the unit test many times and they passed. I will keep investigating this. But I don't think it is a big issue now.
          Hide
          Scott Chen added a comment -

          Sorry for the trouble. I will fix this right now.

          Show
          Scott Chen added a comment - Sorry for the trouble. I will fix this right now.
          Hide
          Ted Yu added a comment -

          I ran TestRowProcessorEndpoint twice based on current trunk and they passed.

          Show
          Ted Yu added a comment - I ran TestRowProcessorEndpoint twice based on current trunk and they passed.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2689 (See https://builds.apache.org/job/HBase-TRUNK/2689/)
          HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) (Revision 1303490)

          Result = FAILURE
          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2689 (See https://builds.apache.org/job/HBase-TRUNK/2689/ ) HBASE-5542 Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() (Scott Chen) (Revision 1303490) Result = FAILURE tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Ted Yu added a comment -

          Integrated patch v3 to TRUNK.

          Thanks for the patch.

          Thanks for the review Lars.

          Show
          Ted Yu added a comment - Integrated patch v3 to TRUNK. Thanks for the patch. Thanks for the review Lars.
          Hide
          Scott Chen added a comment -

          Rebased the patch.

          Show
          Scott Chen added a comment - Rebased the patch.
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Ted, @Lars: Thanks for reviewing this I appreciate it.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          BRANCH
          5542

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Ted, @Lars: Thanks for reviewing this I appreciate it. REVISION DETAIL https://reviews.facebook.net/D2217 BRANCH 5542
          Hide
          Phabricator added a comment -

          lhofhansl has accepted the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          Yeah, let's commit this. I won't get to HBASE-5565 any time soon. Sorry for the hold up.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          BRANCH
          5542

          Show
          Phabricator added a comment - lhofhansl has accepted the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Yeah, let's commit this. I won't get to HBASE-5565 any time soon. Sorry for the hold up. REVISION DETAIL https://reviews.facebook.net/D2217 BRANCH 5542
          Hide
          Ted Yu added a comment -

          I am fine with the latest patch.

          Show
          Ted Yu added a comment - I am fine with the latest patch.
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Lars, @Ted: If HBASE-5542.2.txt looks good to you guys, can we commit it? Then we can move on to HBASE-5565? There is no point to sit here to long. The patch might be stale later.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Lars, @Ted: If HBASE-5542 .2.txt looks good to you guys, can we commit it? Then we can move on to HBASE-5565 ? There is no point to sit here to long. The patch might be stale later. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Ted Yu added a comment -

          That would be done in HBASE-5565, right ?

          Show
          Ted Yu added a comment - That would be done in HBASE-5565 , right ?
          Hide
          Lars Hofhansl added a comment -

          Latest patch looks good. Was wondering whether we should first fold doMiniBatchPut into this, as it will likely require changing some things around.

          Show
          Lars Hofhansl added a comment - Latest patch looks good. Was wondering whether we should first fold doMiniBatchPut into this, as it will likely require changing some things around.
          Hide
          Ted Yu added a comment -

          @Lars:
          Are you Okay with latest patch ?

          Show
          Ted Yu added a comment - @Lars: Are you Okay with latest patch ?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12518431/HBASE-5542.2.txt
          against trunk revision .

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

          +1 tests included. The patch appears to include 4 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 160 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.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1192//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1192//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1192//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/12518431/HBASE-5542.2.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 160 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.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1192//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1192//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1192//console This message is automatically generated.
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          I am also manually upload a patch on JIRA.
          I think reviews.facebook.net doesn't generates the correct patch.
          Please use this one if you need to apply patch.
          https://issues.apache.org/jira/secure/attachment/12518431/HBASE-5542.2.txt

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". I am also manually upload a patch on JIRA. I think reviews.facebook.net doesn't generates the correct patch. Please use this one if you need to apply patch. https://issues.apache.org/jira/secure/attachment/12518431/HBASE-5542.2.txt REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Addressed review comments from Lars, thanks!

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Addressed review comments from Lars, thanks! REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          Thanks for the review.

          >What do you think if I take your patch and try to fit doMiniBatchPut into it and then update the patch accordingly (if needed) - before we commit the patch?

          Sure. Please make it better before you check it in.
          But you will still go through your major work in the other JIRA, right?
          https://issues.apache.org/jira/browse/HBASE-5565
          We will do the review over there. There are too much information in this thread.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:238 Sure. Let me change it to 1 minute.
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4309 Thanks. That's a good catch!
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4286 I see. We can still make remember some state after calling preProcess() and change the output of getRowsToLock(). I like making getRowsToLock() returns the rows rather than preProcess() because it seems more clear to me.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Thanks for the review. >What do you think if I take your patch and try to fit doMiniBatchPut into it and then update the patch accordingly (if needed) - before we commit the patch? Sure. Please make it better before you check it in. But you will still go through your major work in the other JIRA, right? https://issues.apache.org/jira/browse/HBASE-5565 We will do the review over there. There are too much information in this thread. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:238 Sure. Let me change it to 1 minute. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4309 Thanks. That's a good catch! src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4286 I see. We can still make remember some state after calling preProcess() and change the output of getRowsToLock(). I like making getRowsToLock() returns the rows rather than preProcess() because it seems more clear to me. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          This looks pretty nice. It should definitely work for what you set out to do.
          See few comments inline.

          What do you think if I take your patch and try to fit doMiniBatchPut into it and then update the patch accordingly (if needed) - before we commit the patch?

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:238 10 minutes is a very long time to run on the server.
          Was thinking more like a 1 minute default.

          If you think 10 minutes is needed, that's fine too.
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4286 We're getting into details now, but looking at doMiniBatchPut, the failure behavior on locking needs to be defined by the rowprocessors.
          I.e. the preprocess should lock the rows if needed.
          Hmm... But that would take the simplicity away.
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4309 If readOnly() is true, we would not get here, right? So not need to check for readOnly()

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". This looks pretty nice. It should definitely work for what you set out to do. See few comments inline. What do you think if I take your patch and try to fit doMiniBatchPut into it and then update the patch accordingly (if needed) - before we commit the patch? INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:238 10 minutes is a very long time to run on the server. Was thinking more like a 1 minute default. If you think 10 minutes is needed, that's fine too. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4286 We're getting into details now, but looking at doMiniBatchPut, the failure behavior on locking needs to be defined by the rowprocessors. I.e. the preprocess should lock the rows if needed. Hmm... But that would take the simplicity away. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4309 If readOnly() is true, we would not get here, right? So not need to check for readOnly() REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Lars: No problem. Take your time Thanks for the help!

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Lars: No problem. Take your time Thanks for the help! REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          Hey sorry got distracted by other stuff. I promise I'll do a full review tonight.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Hey sorry got distracted by other stuff. I promise I'll do a full review tonight. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Ted Yu added a comment -

          HBASE-5542.txt looks good to me.

          Show
          Ted Yu added a comment - HBASE-5542 .txt looks good to me.
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Lars @Ted: Dose the time bound looks OK now? Let me know if there are things needs to be changed.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Lars @Ted: Dose the time bound looks OK now? Let me know if there are things needs to be changed. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Ted Yu added a comment -

          Thanks for your persistence, Scott.

          Show
          Ted Yu added a comment - Thanks for your persistence, Scott.
          Hide
          Scott Chen added a comment -

          @Ted: I manually upload the patch HBASE-5542.txt.
          I have verified the patch applies to the svn checkout.

          Show
          Scott Chen added a comment - @Ted: I manually upload the patch HBASE-5542 .txt. I have verified the patch applies to the svn checkout.
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Fixed a typo. Sorry.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Fixed a typo. Sorry. REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Log the IOE for easier debugging.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Log the IOE for easier debugging. REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Ted: I think there are some problem with file renaming with reviews.facebook.net.
          My git patch actually works fine. It applies to trunk.

          If this patch doesn't apply again. I will manually upload the patch to JIRA.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Ted: I think there are some problem with file renaming with reviews.facebook.net. My git patch actually works fine. It applies to trunk. If this patch doesn't apply again. I will manually upload the patch to JIRA. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4417 I just check the javadoc.

          It seems that it throws
          TimeoutException, ExecutionException and InterruptedException
          http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/FutureTask.html#get(long, java.util.concurrent.TimeUnit)

          But I can see your point. If the exception is warped too many times, it will be hard to debug.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4417 I just check the javadoc. It seems that it throws TimeoutException, ExecutionException and InterruptedException http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/FutureTask.html#get(long , java.util.concurrent.TimeUnit) But I can see your point. If the exception is warped too many times, it will be hard to debug. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4417 Exception includes IOE.
          Should we check against IOE so that we don't wrap again ?

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4417 Exception includes IOE. Should we check against IOE so that we don't wrap again ? REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Rebase against trunk

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Rebase against trunk REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Use a cached thread pool for the rowProcessor.process() future task.
          Make the default timeout for coprocessor calls 10 minute.

          mutateRowsWithLocks has no timeout and does not use the thread pool.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Use a cached thread pool for the rowProcessor.process() future task. Make the default timeout for coprocessor calls 10 minute. mutateRowsWithLocks has no timeout and does not use the thread pool. REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          I think I should use a thread pool to at lease cache the threads for the time bound case. I will make a quick change and update this.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". I think I should use a thread pool to at lease cache the threads for the time bound case. I will make a quick change and update this. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          See my comment here: https://issues.apache.org/jira/browse/HBASE-5399?focusedCommentId=13228009&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13228009

          The NoSuchMethodException wasn't the root cause for test failure which also happened in TRUNK build #2676.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". See my comment here: https://issues.apache.org/jira/browse/HBASE-5399?focusedCommentId=13228009&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13228009 The NoSuchMethodException wasn't the root cause for test failure which also happened in TRUNK build #2676. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          Here's the stack trace of failure. I don't think it is because of this patch.

          1929eb130380/TestIncrementtestMultiRowMutationMultiThreads/testtable/991d8e4274ee2684b0b8f79e8f31f154/.logs/hlog.1331589169193
          2012-03-12 21:52:49,195 INFO [pool-1-thread-1] wal.HLog(475): getNumCurrentReplicas--HDFS-826 not available; hdfs_out=org.apache.hadoop.fs.FSDataOutputStream@1741361
          java.lang.NoSuchMethodException: org.apache.hadoop.fs.ChecksumFileSystem$ChecksumFSOutputSummer.getNumCurrentReplicas()
          at java.lang.Class.getDeclaredMethod(Class.java:1937)
          at org.apache.hadoop.hbase.regionserver.wal.HLog.getGetNumCurrentReplicas(HLog.java:460)
          at org.apache.hadoop.hbase.regionserver.wal.HLog.<init>(HLog.java:443)
          at org.apache.hadoop.hbase.regionserver.wal.HLog.<init>(HLog.java:341)
          at org.apache.hadoop.hbase.regionserver.HRegion.createHRegion(HRegion.java:3596)
          at org.apache.hadoop.hbase.regionserver.HRegion.createHRegion(HRegion.java:3563)
          at org.apache.hadoop.hbase.regionserver.TestAtomicOperation.initHRegion(TestAtomicOperation.java:214)
          at org.apache.hadoop.hbase.regionserver.TestAtomicOperation.initHRegion(TestAtomicOperation.java:197)
          at org.apache.hadoop.hbase.regionserver.TestAtomicOperation.testMultiRowMutationMultiThreads(TestAtomicOperation.java:341)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at junit.framework.TestCase.runTest(TestCase.java:168)
          at junit.framework.TestCase.runBare(TestCase.java:134)
          at junit.framework.TestResult$1.protect(TestResult.java:110)
          at junit.framework.TestResult.runProtected(TestResult.java:128)
          at junit.framework.TestResult.run(TestResult.java:113)
          at junit.framework.TestCase.run(TestCase.java:124)
          at junit.framework.TestSuite.runTest(TestSuite.java:243)
          at junit.framework.TestSuite.run(TestSuite.java:238)
          at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:83)
          at org.junit.runners.Suite.runChild(Suite.java:128)
          at org.junit.runners.Suite.runChild(Suite.java:24)
          at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
          at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441)
          at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
          at java.util.concurrent.FutureTask.run(FutureTask.java:138)
          at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
          at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
          at java.lang.Thread.run(Thread.java:662)

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Here's the stack trace of failure. I don't think it is because of this patch. 1929eb130380/TestIncrementtestMultiRowMutationMultiThreads/testtable/991d8e4274ee2684b0b8f79e8f31f154/.logs/hlog.1331589169193 2012-03-12 21:52:49,195 INFO [pool-1-thread-1] wal.HLog(475): getNumCurrentReplicas-- HDFS-826 not available; hdfs_out=org.apache.hadoop.fs.FSDataOutputStream@1741361 java.lang.NoSuchMethodException: org.apache.hadoop.fs.ChecksumFileSystem$ChecksumFSOutputSummer.getNumCurrentReplicas() at java.lang.Class.getDeclaredMethod(Class.java:1937) at org.apache.hadoop.hbase.regionserver.wal.HLog.getGetNumCurrentReplicas(HLog.java:460) at org.apache.hadoop.hbase.regionserver.wal.HLog.<init>(HLog.java:443) at org.apache.hadoop.hbase.regionserver.wal.HLog.<init>(HLog.java:341) at org.apache.hadoop.hbase.regionserver.HRegion.createHRegion(HRegion.java:3596) at org.apache.hadoop.hbase.regionserver.HRegion.createHRegion(HRegion.java:3563) at org.apache.hadoop.hbase.regionserver.TestAtomicOperation.initHRegion(TestAtomicOperation.java:214) at org.apache.hadoop.hbase.regionserver.TestAtomicOperation.initHRegion(TestAtomicOperation.java:197) at org.apache.hadoop.hbase.regionserver.TestAtomicOperation.testMultiRowMutationMultiThreads(TestAtomicOperation.java:341) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:168) at junit.framework.TestCase.runBare(TestCase.java:134) at junit.framework.TestResult$1.protect(TestResult.java:110) at junit.framework.TestResult.runProtected(TestResult.java:128) at junit.framework.TestResult.run(TestResult.java:113) at junit.framework.TestCase.run(TestCase.java:124) at junit.framework.TestSuite.runTest(TestSuite.java:243) at junit.framework.TestSuite.run(TestSuite.java:238) at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:83) at org.junit.runners.Suite.runChild(Suite.java:128) at org.junit.runners.Suite.runChild(Suite.java:24) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441) at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303) at java.util.concurrent.FutureTask.run(FutureTask.java:138) at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908) at java.lang.Thread.run(Thread.java:662) REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          The latest unit test failed. I think it is caused by this patch. I will fix it.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". The latest unit test failed. I think it is caused by this patch. I will fix it. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Addressed comments from Lars, Thanks!
          Also added a unit test for the timeout case.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Addressed comments from Lars, Thanks! Also added a unit test for the timeout case. REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Lars: I agree. The internal calls should never create threads for this. I will make the change.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:485 The timeout is set here.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Lars: I agree. The internal calls should never create threads for this. I will make the change. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:485 The timeout is set here. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          In the latest patch I can't find rowProcessorTimeout being set anywhere.

          What I had in mind was this:
          We are now using this for "internal" operations (such as mutateRow) and "external" operations (those created by users). The internal operations should not be creating new threads. External operations either could always time bound or it could be up to the implementer.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". In the latest patch I can't find rowProcessorTimeout being set anywhere. What I had in mind was this: We are now using this for "internal" operations (such as mutateRow) and "external" operations (those created by users). The internal operations should not be creating new threads. External operations either could always time bound or it could be up to the implementer. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Addressed Ted's review comments, Thanks!

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Addressed Ted's review comments, Thanks! REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java:53 If "hbase.hregion.row.processor.timeout" carries positive value, new Thread would be spawned for mutateRowsWithLocks().
          Does this satisfy Lars' comment @ 12/Mar/12 16:33 ?

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java:53 If "hbase.hregion.row.processor.timeout" carries positive value, new Thread would be spawned for mutateRowsWithLocks(). Does this satisfy Lars' comment @ 12/Mar/12 16:33 ? REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java:53 Ted: there is a return statement here. I will add a comment saying short circuit for the nagative timeout case.
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java:93 goodcatch. Thanks!

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java:53 Ted: there is a return statement here. I will add a comment saying short circuit for the nagative timeout case. src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java:93 goodcatch. Thanks! REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java:93 Should read 'switch off'
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java:51 If positive timeout is specified, new Thread would be spawned for both cases.
          Is that desirable ?

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java:93 Should read 'switch off' src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java:51 If positive timeout is specified, new Thread would be spawned for both cases. Is that desirable ? REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Make the defaut timeout infinite (no extra thread in this case)

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Make the defaut timeout infinite (no extra thread in this case) REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Ted: I will also make the time bound default value to be large.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Ted: I will also make the time bound default value to be large. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @lhofhansl: I have created a jira for you
          https://issues.apache.org/jira/browse/HBASE-5565
          Thank you for volunteer on working on it.

          I will make the time bound optional. Will update the patch soon.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @lhofhansl: I have created a jira for you https://issues.apache.org/jira/browse/HBASE-5565 Thank you for volunteer on working on it. I will make the time bound optional. Will update the patch soon. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Scott: Yes, let's use another jira for doMiniBatchPut.
          For the timebound logic, at the very least there has to be an option to not do that for mutateRowsWithLocks, as spawning another thread for that would be too expensive.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Scott: Yes, let's use another jira for doMiniBatchPut. For the timebound logic, at the very least there has to be an option to not do that for mutateRowsWithLocks, as spawning another thread for that would be too expensive. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Ted Yu added a comment -

          I think we should keep time bound whose default value can be large.

          Show
          Ted Yu added a comment - I think we should keep time bound whose default value can be large.
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Lars: Sorry. I also missed your previous reply in the JIRA.
          The JIRA is kind of flooded by HadoopQA and Phabricator and becomes harder to find things.

          For the doMiniBatchPut, I think it will be better if we can do it in another patch. It will be easier to review and actually move faster.

          About the time bound, I think it makes sense to leave it to the application to decide.
          What do you think about this, Ted?

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Lars: Sorry. I also missed your previous reply in the JIRA. The JIRA is kind of flooded by HadoopQA and Phabricator and becomes harder to find things. For the doMiniBatchPut, I think it will be better if we can do it in another patch. It will be easier to review and actually move faster. About the time bound, I think it makes sense to leave it to the application to decide. What do you think about this, Ted? REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Addressed Ted's review comment.
          Add one more unit test for testing multiple row locking.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Addressed Ted's review comment. Add one more unit test for testing multiple row locking. REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          MultiRowProcessor.java no longer exists.
          Javadoc still refers to it. This would create confusion.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java:42 This doesn't match class name.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". MultiRowProcessor.java no longer exists. Javadoc still refers to it. This would create confusion. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java:42 This doesn't match class name. REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc has requested a review of the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - sc has requested a review of the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          One more typo

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA One more typo REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          I made several changes in this update.

          1. Add the post and pre process hook as suggested by Lars
          2. RowProcessorProtocol and BaseRowProcessorEndpoint
          3. Some refactor and renaming

          I add back the method RowProcessorProtocol.process(RowProcessor).
          I did this because I figured out the user can make their RowProcessor implementations as the inner classes of the endpoint.
          This way RowProcessor can be class-loaded with the endpoint.
          I have documented in the javadoc.

          The example can be found in the unit test.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA I made several changes in this update. 1. Add the post and pre process hook as suggested by Lars 2. RowProcessorProtocol and BaseRowProcessorEndpoint 3. Some refactor and renaming I add back the method RowProcessorProtocol.process(RowProcessor). I did this because I figured out the user can make their RowProcessor implementations as the inner classes of the endpoint. This way RowProcessor can be class-loaded with the endpoint. I have documented in the javadoc. The example can be found in the unit test. REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Fixed some typo

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java
          src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Fixed some typo REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessorEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessorProtocol.java src/main/java/org/apache/hadoop/hbase/regionserver/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestRowProcessorEndpoint.java
          Hide
          Lars Hofhansl added a comment -

          If you like I can take a shot at refactoring doMiniBatchPut with the new approach once you're done with mutateRowsWithLocks and processRowsWithLocks.
          Of course if you prefer to do the whole thing that's fine too

          Show
          Lars Hofhansl added a comment - If you like I can take a shot at refactoring doMiniBatchPut with the new approach once you're done with mutateRowsWithLocks and processRowsWithLocks. Of course if you prefer to do the whole thing that's fine too
          Hide
          Lars Hofhansl added a comment -

          @Scott: Sorry missed your earlier comment. isAtomic would work.
          I wonder if we'd get more flexibility if we leave this logic in the row processor.
          In the case of doMiniBatchPut, the row processor could maintain batchOp as internal state and use that in all of pre/post/process.

          Re: your last comment. Thanks for bearing with me. As I said, this is important (and a bit tricky) to get right. Thank you very much for working on it!

          I hope we won't find any holes in the latest approach.

          Actually one more thing I was wondering: For the core operations, it makes little sense to timebound this with an extra thread. It would be nice if there was some option to enforce a timebound or leave it up to the implementer to guarantee it won't take too long.
          In fact I would probably leave the time bounding completely to the custom code.

          Show
          Lars Hofhansl added a comment - @Scott: Sorry missed your earlier comment. isAtomic would work. I wonder if we'd get more flexibility if we leave this logic in the row processor. In the case of doMiniBatchPut, the row processor could maintain batchOp as internal state and use that in all of pre/post/process. Re: your last comment. Thanks for bearing with me. As I said, this is important (and a bit tricky) to get right. Thank you very much for working on it! I hope we won't find any holes in the latest approach. Actually one more thing I was wondering: For the core operations, it makes little sense to timebound this with an extra thread. It would be nice if there was some option to enforce a timebound or leave it up to the implementer to guarantee it won't take too long. In fact I would probably leave the time bounding completely to the custom code.
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Lars: The approach looks solid. I will start implement it. Thanks!
          It's been great fun working with you!

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          BRANCH
          5542-new-file

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Lars: The approach looks solid. I will start implement it. Thanks! It's been great fun working with you! REVISION DETAIL https://reviews.facebook.net/D2217 BRANCH 5542-new-file
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          @Scott: Yes, it would make the API more complicated, which is not desirable.

          Hmm... I cannot see any way around it for the general case, though. There are going to be operations that need to do some stuff without the locks held in the beginning and some stuff in the end.
          On the other hand, if an operation has not pre/post tasks it would just not implement them (or implement no-ops)

          So the overall flow of processRowsWithLocks would be something like:
          1. startRegionOperation
          2. -> run pre tasks (maybe these should return the rows to lock?)
          3. acquire locks and start mvcc transaction
          4. -> run the main operation... returns the KVs to use
          5. apply the KVs to memstore and WAL (or should the main op be required to apply the KVs?)
          6. unlock
          7. sync wal
          8. roll mvcc
          9. -> run post tasks
          (10. cleanup if needed)

          -> denotes a callout to the row processor

          Let's be careful that we get this right. It's also interesting to see whether we can fold doMiniBatchPut (or at least some aspects of it) into this. With the 3 phases it looks like this should possible (I'm happy to look at that separately)

          Thanks for bearing with us Scott! This is awesome stuff.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          BRANCH
          5542-new-file

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". @Scott: Yes, it would make the API more complicated, which is not desirable. Hmm... I cannot see any way around it for the general case, though. There are going to be operations that need to do some stuff without the locks held in the beginning and some stuff in the end. On the other hand, if an operation has not pre/post tasks it would just not implement them (or implement no-ops) So the overall flow of processRowsWithLocks would be something like: 1. startRegionOperation 2. -> run pre tasks (maybe these should return the rows to lock?) 3. acquire locks and start mvcc transaction 4. -> run the main operation... returns the KVs to use 5. apply the KVs to memstore and WAL (or should the main op be required to apply the KVs?) 6. unlock 7. sync wal 8. roll mvcc 9. -> run post tasks (10. cleanup if needed) -> denotes a callout to the row processor Let's be careful that we get this right. It's also interesting to see whether we can fold doMiniBatchPut (or at least some aspects of it) into this. With the 3 phases it looks like this should possible (I'm happy to look at that separately) Thanks for bearing with us Scott! This is awesome stuff. REVISION DETAIL https://reviews.facebook.net/D2217 BRANCH 5542-new-file
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java:85 good point
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4362 goodcatch.

          Should we add something like preProcess(), postProcess() to RowProcessor? This somehow makes the API more complicated.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          BRANCH
          5542-new-file

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java:85 good point src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4362 goodcatch. Should we add something like preProcess(), postProcess() to RowProcessor? This somehow makes the API more complicated. REVISION DETAIL https://reviews.facebook.net/D2217 BRANCH 5542-new-file
          Hide
          Scott Chen added a comment -

          @Lars: It seems to me that the major difference in doMiniBatchPut is that it gets the locks with non-block option and process the locked rows. It remembers a index of processed row and keeps continuing.

          How about making MultiRowProcessor having an atomic option?

          interface MultiRowProcessor {
          
            boolean isAtomic();
          
            Collection<byte[]> rowsToLock();
          
            // need to tell processor which rows to process in the non-atomic mode
            process(Collection<byte[]> rowsLocked); 
          
          }
          

          In HRegion, we check if the processor is atomic.
          If it is not, we get as many lock as possible and process them in a loop.

          HRegion.processRowsWithLocks(MultiRowProcessor processor) {
          
            while (done) {
              if (processor.isAtomic()) {
                // get all locks with blocking option
              } else {
                // get as many locks with non-blocking option
              }
              processor.process(lockedRows);
              
              if (/*we process all the rows*/) {
                done = true;
              }
            }
          
          }
          

          What do you think?

          Show
          Scott Chen added a comment - @Lars: It seems to me that the major difference in doMiniBatchPut is that it gets the locks with non-block option and process the locked rows. It remembers a index of processed row and keeps continuing. How about making MultiRowProcessor having an atomic option? interface MultiRowProcessor { boolean isAtomic(); Collection< byte []> rowsToLock(); // need to tell processor which rows to process in the non-atomic mode process(Collection< byte []> rowsLocked); } In HRegion, we check if the processor is atomic. If it is not, we get as many lock as possible and process them in a loop. HRegion.processRowsWithLocks(MultiRowProcessor processor) { while (done) { if (processor.isAtomic()) { // get all locks with blocking option } else { // get as many locks with non-blocking option } processor.process(lockedRows); if (/*we process all the rows*/) { done = true ; } } } What do you think?
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4362 Hmm... That means we're now running the coprocessor post host after the region operation closed.
          It almost seems that the RowProcessor should have three phases:
          1. A pre phase (without locks and mvcc, but with the WALEdit). The prephase could abort the operation
          2. the op itself
          3. a post operation (after locks are released, mvcc is rolled forward)

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          BRANCH
          5542-new-file

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4362 Hmm... That means we're now running the coprocessor post host after the region operation closed. It almost seems that the RowProcessor should have three phases: 1. A pre phase (without locks and mvcc, but with the WALEdit). The prephase could abort the operation 2. the op itself 3. a post operation (after locks are released, mvcc is rolled forward) REVISION DETAIL https://reviews.facebook.net/D2217 BRANCH 5542-new-file
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Rebase with HBASE-5541

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Rebase with HBASE-5541 REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Scott Chen added a comment -

          @Stack: Thanks for the review! We will get this done

          Show
          Scott Chen added a comment - @Stack: Thanks for the review! We will get this done
          Hide
          Scott Chen added a comment -

          @Ted: Thanks for the help. I will rebase this one right away.

          @Lars: I just see your work in HBASE-5541. That's pretty nice.
          I will double check with it to make sure I am also doing the right thing in this one.
          Thanks!

          Show
          Scott Chen added a comment - @Ted: Thanks for the help. I will rebase this one right away. @Lars: I just see your work in HBASE-5541 . That's pretty nice. I will double check with it to make sure I am also doing the right thing in this one. Thanks!
          Hide
          stack added a comment -

          I took a look. Patch looks good. I think this kinda work is really important to do – now while its fresh in everyone's minds whats going on here – if we're not to let the mess get out of hand. Thanks Scott.

          Yeah Lars, this is your old stomping ground. Could do w/ a review by you.

          Show
          stack added a comment - I took a look. Patch looks good. I think this kinda work is really important to do – now while its fresh in everyone's minds whats going on here – if we're not to let the mess get out of hand. Thanks Scott. Yeah Lars, this is your old stomping ground. Could do w/ a review by you.
          Hide
          Lars Hofhansl added a comment - - edited

          Can we let this sit for a bit? I would like to spend some time thinking about this. doMiniBatchPut almost follows the same logic, should be able to unify that code as well. It's not needed for 0.94, so it's not time critical.

          Thanks for working on this Scott!

          Show
          Lars Hofhansl added a comment - - edited Can we let this sit for a bit? I would like to spend some time thinking about this. doMiniBatchPut almost follows the same logic, should be able to unify that code as well. It's not needed for 0.94, so it's not time critical. Thanks for working on this Scott!
          Hide
          Lars Hofhansl added a comment -

          Let me do a pass through it before you commit.

          Show
          Lars Hofhansl added a comment - Let me do a pass through it before you commit.
          Hide
          Phabricator added a comment -

          tedyu has accepted the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          If tests pass.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowProcessor.java:31 Should we mention that the rows are inside one region ?

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          BRANCH
          5542-new-file

          Show
          Phabricator added a comment - tedyu has accepted the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". If tests pass. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowProcessor.java:31 Should we mention that the rows are inside one region ? REVISION DETAIL https://reviews.facebook.net/D2217 BRANCH 5542-new-file
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Addressed Ted's review comments, Thanks.

          Sorry for dumping the RowProcessor API almost immediately.
          After created MultiRowProcessor, I figured out that we don't need the RowProcessor interface anymore.
          Better to get ride of it earlier.

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Addressed Ted's review comments, Thanks. Sorry for dumping the RowProcessor API almost immediately. After created MultiRowProcessor, I figured out that we don't need the RowProcessor interface anymore. Better to get ride of it earlier. REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Ted Yu added a comment -

          Please refresh patch as Lars recently integrated HBASE-5541

          Show
          Ted Yu added a comment - Please refresh patch as Lars recently integrated HBASE-5541
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".

          Thanks for the fast turn-around.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java:35 Add javadoc for this class, please.
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java:85 We can store m.getWriteToWAL() in a boolean before entering the loop and use the boolean variable, right ?

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Thanks for the fast turn-around. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java:35 Add javadoc for this class, please. src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java:85 We can store m.getWriteToWAL() in a boolean before entering the loop and use the boolean variable, right ? REVISION DETAIL https://reviews.facebook.net/D2217
          Hide
          Phabricator added a comment -

          sc requested code review of "HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()".
          Reviewers: tedyu, lhofhansl, JIRA

          Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()

          Task ID: #

          Blame Rev:

          mutateRowsWithLocks() does atomic mutations on multiple rows.
          processRow() does atomic read-modify-writes on a single row.

          It will be useful to generalize both and have a
          processRowsWithLocks() that does atomic read-modify-writes on multiple rows.

          This also helps reduce some redundancy in the codes.

          TEST PLAN
          Revert Plan:

          Tags:

          REVISION DETAIL
          https://reviews.facebook.net/D2217

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java

          MANAGE HERALD DIFFERENTIAL RULES
          https://reviews.facebook.net/herald/view/differential/

          WHY DID I GET THIS EMAIL?
          https://reviews.facebook.net/herald/transcript/4851/

          Tip: use the X-Herald-Rules header to filter Herald messages in your client.

          Show
          Phabricator added a comment - sc requested code review of " HBASE-5542 [jira] Unify HRegion.mutateRowsWithLocks() and HRegion.processRow()". Reviewers: tedyu, lhofhansl, JIRA Unify HRegion.mutateRowsWithLocks() and HRegion.processRow() Task ID: # Blame Rev: mutateRowsWithLocks() does atomic mutations on multiple rows. processRow() does atomic read-modify-writes on a single row. It will be useful to generalize both and have a processRowsWithLocks() that does atomic read-modify-writes on multiple rows. This also helps reduce some redundancy in the codes. TEST PLAN Revert Plan: Tags: REVISION DETAIL https://reviews.facebook.net/D2217 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowMutationProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/MultiRowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/4851/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.

            People

            • Assignee:
              Scott Chen
              Reporter:
              Scott Chen
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development