HBase
  1. HBase
  2. HBASE-5515

Add a processRow API that supports atomic multiple reads and writes on a row

    Details

    • Type: New Feature New Feature
    • 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

      We have modified HRegion.java internally to do some atomic row processing. It will be nice to have a plugable API for this.

      1. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.1.patch
        24 kB
        Phabricator
      2. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.2.patch
        25 kB
        Phabricator
      3. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.3.patch
        25 kB
        Phabricator
      4. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.4.patch
        26 kB
        Phabricator
      5. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.5.patch
        26 kB
        Phabricator
      6. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.6.patch
        28 kB
        Phabricator
      7. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.7.patch
        28 kB
        Phabricator
      8. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.8.patch
        28 kB
        Phabricator
      9. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.9.patch
        26 kB
        Phabricator
      10. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.10.patch
        26 kB
        Phabricator
      11. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.11.patch
        26 kB
        Phabricator
      12. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.12.patch
        26 kB
        Phabricator
      13. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.13.patch
        26 kB
        Phabricator
      14. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.14.patch
        26 kB
        Phabricator
      15. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.15.patch
        24 kB
        Phabricator
      16. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.16.patch
        27 kB
        Phabricator
      17. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.17.patch
        36 kB
        Phabricator
      18. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.18.patch
        34 kB
        Phabricator
      19. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.19.patch
        32 kB
        Phabricator
      20. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.20.patch
        23 kB
        Phabricator
      21. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.21.patch
        22 kB
        Phabricator
      22. ASF.LICENSE.NOT.GRANTED--HBASE-5515.D2067.22.patch
        22 kB
        Phabricator

        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-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

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

          To: tedyu, dhruba, JIRA, sc
          Cc: lhofhansl, jyates

          Show
          Phabricator added a comment - sc has closed the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". REVISION DETAIL https://reviews.facebook.net/D2067 To: tedyu, dhruba, JIRA, sc Cc: lhofhansl, jyates
          Hide
          Scott Chen added a comment -

          @Lars: Yes, 0.96 sounds good to me.

          Show
          Scott Chen added a comment - @Lars: Yes, 0.96 sounds good to me.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #132 (See https://builds.apache.org/job/HBase-TRUNK-security/132/)
          HBASE-5515 Add a processRow API that supports atomic multiple reads and writes on a row (Scott Chen) (Revision 1298533)

          Result = FAILURE
          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #132 (See https://builds.apache.org/job/HBase-TRUNK-security/132/ ) HBASE-5515 Add a processRow API that supports atomic multiple reads and writes on a row (Scott Chen) (Revision 1298533) Result = FAILURE tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2674 (See https://builds.apache.org/job/HBase-TRUNK/2674/)
          HBASE-5515 Add a processRow API that supports atomic multiple reads and writes on a row (Scott Chen) (Revision 1298533)

          Result = FAILURE
          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2674 (See https://builds.apache.org/job/HBase-TRUNK/2674/ ) HBASE-5515 Add a processRow API that supports atomic multiple reads and writes on a row (Scott Chen) (Revision 1298533) Result = FAILURE tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Lars Hofhansl added a comment -

          Yeah with "0.96" above I mean trunk.

          Show
          Lars Hofhansl added a comment - Yeah with "0.96" above I mean trunk.
          Hide
          dhruba borthakur added a comment -

          My vote is to put it into hbase-trunk only (and not to backport to 0.94)

          Show
          dhruba borthakur added a comment - My vote is to put it into hbase-trunk only (and not to backport to 0.94)
          Hide
          Lars Hofhansl added a comment -

          I think this should go into 0.96. We can backport later if the need arises.
          Scott, what do you think?

          Show
          Lars Hofhansl added a comment - I think this should go into 0.96. We can backport later if the need arises. Scott, what do you think?
          Hide
          Ted Yu added a comment -

          Integrated to TRUNK.

          Thanks for the patch, Scott.

          Thanks for the review Lars and Jesse.

          Show
          Ted Yu added a comment - Integrated to TRUNK. Thanks for the patch, Scott. Thanks for the review Lars and Jesse.
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          @Lars, @Ted, @Jesse: Thanks for the reviews You guys helped a lot.
          I created https://issues.apache.org/jira/browse/HBASE-5542 for the unify work.

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

          BRANCH
          5515

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". @Lars, @Ted, @Jesse: Thanks for the reviews You guys helped a lot. I created https://issues.apache.org/jira/browse/HBASE-5542 for the unify work. REVISION DETAIL https://reviews.facebook.net/D2067 BRANCH 5515
          Hide
          Ted Yu added a comment -

          @Lars:
          Do you think this feature should go to 0.94 ?

          Please assign 'Fix Version' accordingly.

          Show
          Ted Yu added a comment - @Lars: Do you think this feature should go to 0.94 ? Please assign 'Fix Version' accordingly.
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          +1... Ship it.

          We should unify HRegion.mutateRowWithLocks and HRegion.processRow, but let's do that as a separate issue.

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

          BRANCH
          5515

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". +1... Ship it. We should unify HRegion.mutateRowWithLocks and HRegion.processRow, but let's do that as a separate issue. REVISION DETAIL https://reviews.facebook.net/D2067 BRANCH 5515
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12517584/HBASE-5515.D2067.22.patch
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -129 warning messages.

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

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1135//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1135//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1135//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/12517584/HBASE-5515.D2067.22.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -129 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 155 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1135//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1135//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1135//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12517582/HBASE-5515.D2067.21.patch
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -129 warning messages.

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

          -1 findbugs. The patch appears to introduce 155 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.TestReplicationPeer
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestImportTsv

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1134//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1134//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1134//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/12517582/HBASE-5515.D2067.21.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -129 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 155 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.TestReplicationPeer org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestImportTsv Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1134//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1134//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1134//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12517577/HBASE-5515.D2067.20.patch
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -129 warning messages.

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

          -1 findbugs. The patch appears to introduce 155 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/1133//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1133//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1133//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/12517577/HBASE-5515.D2067.20.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -129 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 155 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/1133//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1133//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1133//console This message is automatically generated.
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Fixed the indentation and remove an unused method

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

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

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Fixed the indentation and remove an unused method REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          tedyu has accepted the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          If tests pass.

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

          BRANCH
          5515

          Show
          Phabricator added a comment - tedyu has accepted the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". If tests pass. REVISION DETAIL https://reviews.facebook.net/D2067 BRANCH 5515
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Remove the redundant abstract keyword

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

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

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Remove the redundant abstract keyword REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          I tried making ProcessorRowEnpoint endpoint an abstract class. The difficulty is that when creating ProcessRowProtocal you can not pass parameter in the constructor. So the parameter has to be passed by the actually processRow() call. You can use generic

          ProcessRowProtocol

          { <T, S> T processRow(S input); }

          But this is kind of ugly. And you will need to have the actual S in the class path also. This is not good for the same reason you don't what RowProcessor as a separate class.

          So I just leave the users to do their own CoprocessorProtocol and CoprocessorEndpoint just like the example in the unit test.

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". I tried making ProcessorRowEnpoint endpoint an abstract class. The difficulty is that when creating ProcessRowProtocal you can not pass parameter in the constructor. So the parameter has to be passed by the actually processRow() call. You can use generic ProcessRowProtocol { <T, S> T processRow(S input); } But this is kind of ugly. And you will need to have the actual S in the class path also. This is not good for the same reason you don't what RowProcessor as a separate class. So I just leave the users to do their own CoprocessorProtocol and CoprocessorEndpoint just like the example in the unit test. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:53 goodcatch

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:53 goodcatch REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:53 Is abstract keyword needed ?

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

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:53 Is abstract keyword needed ? REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Addressed jyates's review comments, thanks.

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

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

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Addressed jyates's review comments, thanks. REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          @jyates: Thanks for the review comments. I think what you and Lars think makes sense. I guess I started this patch by using RowProcessor a parameter of HTable. So I feel RowProcessor should be a parameter. But with the Coprocessor case, it seems what you guys think makes more sense.

          I will update this patch soon.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:52 The abstract method approach is a good idea. Thanks!

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". @jyates: Thanks for the review comments. I think what you and Lars think makes sense. I guess I started this patch by using RowProcessor a parameter of HTable. So I feel RowProcessor should be a parameter. But with the Coprocessor case, it seems what you guys think makes more sense. I will update this patch soon. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:52 The abstract method approach is a good idea. Thanks! REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          jyates has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Still seems a bit bloated. I don't know what we really get from having the RowProcessor not be an endpoint in itself. A lot of the work here could be handled by just having an abstract class that implements CoprocessorEndpoint and is subclasses by actual RowProcessor implementations. This would also help keep the API more condensed. More thoughts inline on how that would work.

          Otherwise, seems solid.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:34 This whole process seems a bit roundabout. You make an endpoint that then takes the writable which immediately then just calls the hregion to process that processor.... have to agree with Lars that the endpoint should in fact just be the rowProcesssor - you would have the FriendsOfFriendsProcessor just be a coprocessor that you turn on for your table.
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:52 If you wanted to make it easier for people to implement their own multi-action processing, then (going back to my comment above) you can make the RowProcessorEnpoint just have an abstract method that people use to implement their own processing.

          This abstract method would actually be called inside of the 'process' method that would take care of getting the row lock, etc and leaving just the processing for the actual implementation (which could return an iterable) and then push them back to the back.

          Some work would be involved in this approach, but it would be easily extractable into the stuff from HBASE-5529
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4303 This seems like a lot of copying from other parts in the code - maybe it can be abstracted out? This might be something for the next patch for the MultiRowProcessor stuff

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

          Show
          Phabricator added a comment - jyates has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Still seems a bit bloated. I don't know what we really get from having the RowProcessor not be an endpoint in itself. A lot of the work here could be handled by just having an abstract class that implements CoprocessorEndpoint and is subclasses by actual RowProcessor implementations. This would also help keep the API more condensed. More thoughts inline on how that would work. Otherwise, seems solid. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:34 This whole process seems a bit roundabout. You make an endpoint that then takes the writable which immediately then just calls the hregion to process that processor.... have to agree with Lars that the endpoint should in fact just be the rowProcesssor - you would have the FriendsOfFriendsProcessor just be a coprocessor that you turn on for your table. src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:52 If you wanted to make it easier for people to implement their own multi-action processing, then (going back to my comment above) you can make the RowProcessorEnpoint just have an abstract method that people use to implement their own processing. This abstract method would actually be called inside of the 'process' method that would take care of getting the row lock, etc and leaving just the processing for the actual implementation (which could return an iterable) and then push them back to the back. Some work would be involved in this approach, but it would be easily extractable into the stuff from HBASE-5529 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4303 This seems like a lot of copying from other parts in the code - maybe it can be abstracted out? This might be something for the next patch for the MultiRowProcessor stuff REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Ted Yu added a comment -

          @Andy, @Gary:
          Do you want to take a look at the latest patch ?

          Show
          Ted Yu added a comment - @Andy, @Gary: Do you want to take a look at the 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/12517356/HBASE-5515.D2067.19.patch
          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 appears to have generated -129 warning messages.

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

          -1 findbugs. The patch appears to introduce 155 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.TestHFileOutputFormat
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestImportTsv

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1127//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1127//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1127//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/12517356/HBASE-5515.D2067.19.patch 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 appears to have generated -129 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 155 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.TestHFileOutputFormat org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestImportTsv Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1127//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1127//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1127//console This message is automatically generated.
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Addressed Ted's review comments, thanks!
          Also remove more repeated codes in unit tests.

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowWithCustomMadeProtocol.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Addressed Ted's review comments, thanks! Also remove more repeated codes in unit tests. REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowWithCustomMadeProtocol.java
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:36 ProcessRowEndpoint.class should be used here.
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:48 Should read 'table where the row to process resides'
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowWithCustomMadeProtocol.java:221 walEdit (singular form) would be a better name for the last parameter.

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

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:36 ProcessRowEndpoint.class should be used here. src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:48 Should read 'table where the row to process resides' src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowWithCustomMadeProtocol.java:221 walEdit (singular form) would be a better name for the last parameter. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:72 This just do a getScanner(scan) and then scan for one row. And yes, I add this just to get access to HRegion.getScanner().

          Coprocessor is really useful. Thanks for all the review comments. I learned a lot about coprocessor after this patch

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:72 This just do a getScanner(scan) and then scan for one row. And yes, I add this just to get access to HRegion.getScanner(). Coprocessor is really useful. Thanks for all the review comments. I learned a lot about coprocessor after this patch REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Looks good.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:72 What does this do on top of a "normal" scanner?
          Is it that a RowProcessor implementation does not have access to the HRegion and hence cannot get a scanner?
          (One more reason for using coprocessor endpoints, as they can just get the HRegion through the coprocessor environment. )
          I.e. the coprocessor can just call HRegion.getScanner(scan).

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

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Looks good. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:72 What does this do on top of a "normal" scanner? Is it that a RowProcessor implementation does not have access to the HRegion and hence cannot get a scanner? (One more reason for using coprocessor endpoints, as they can just get the HRegion through the coprocessor environment. ) I.e. the coprocessor can just call HRegion.getScanner(scan). REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Remove some repeated codes in unit tests

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowWithCustomMadeProtocol.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Remove some repeated codes in unit tests REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowWithCustomMadeProtocol.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          @Lars:
          "They can be loaded dynamically from a jar on HDFS and per table, or statically and globally for all tables."
          This is really cool!

          I made a sight change to HRegion.processRow() so that it takes a RowProcessor interface (instead of the BaseRowProcessor class)
          Now people can still define their own CorocessorProtocol that implements RowProcessor and have the ability to dynamically load this class as you described.
          I wrote a unit test TestProcessRowWithCustomMadeProtocol.java to verify that use case.

          But if this is cool with you, I sill like to keep the ProcessRowProtocol and ProcessRowEndpoint because that's a convenient entry point for most users.

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowWithCustomMadeProtocol.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA @Lars: "They can be loaded dynamically from a jar on HDFS and per table, or statically and globally for all tables." This is really cool! I made a sight change to HRegion.processRow() so that it takes a RowProcessor interface (instead of the BaseRowProcessor class) Now people can still define their own CorocessorProtocol that implements RowProcessor and have the ability to dynamically load this class as you described. I wrote a unit test TestProcessRowWithCustomMadeProtocol.java to verify that use case. But if this is cool with you, I sill like to keep the ProcessRowProtocol and ProcessRowEndpoint because that's a convenient entry point for most users. REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowWithCustomMadeProtocol.java
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          @Scott: FriendsOfFriendsProtocol would be known to the client and FriendsOfFriendsEndpoint to the server.
          You can always add a tiny client side wrapper to make it more convenient.

          It's cool if you want to do it differently.
          But keep in mind that a lot of effort went into getting classloading for coprocessors right. They can be loaded dynamically from a jar on HDFS and per table, or statically and globally for all tables.
          For example you could deploy new row processing logic without touching the servers at all (by loading a jar from HDFS).
          That is actually something I do not like about filters, to use a new filter it needs to be deployed at every regionserver and requires a rolling restart of the cluster. Coprocessors nicely solve this problems.

          With the current approach the row processing implementation would need to be deployed outside of the coprocessor framework and always require a cluster restart.

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

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". @Scott: FriendsOfFriendsProtocol would be known to the client and FriendsOfFriendsEndpoint to the server. You can always add a tiny client side wrapper to make it more convenient. It's cool if you want to do it differently. But keep in mind that a lot of effort went into getting classloading for coprocessors right. They can be loaded dynamically from a jar on HDFS and per table, or statically and globally for all tables. For example you could deploy new row processing logic without touching the servers at all (by loading a jar from HDFS). That is actually something I do not like about filters, to use a new filter it needs to be deployed at every regionserver and requires a rolling restart of the cluster. Coprocessors nicely solve this problems. With the current approach the row processing implementation would need to be deployed outside of the coprocessor framework and always require a cluster restart. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Forgot to include BaseRowProcessor.java

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Forgot to include BaseRowProcessor.java REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Addressed Lars and Ted's comments

          @tedyu: I changed reference count to 31 and long count to 6.

          This is because we have added the following two things
          long rowProcessorTimeout
          RowProcessor.RowScanner rowScanner

          @lhofhansl: I have made most of the changes you suggested. Thanks!

          I tried to put RowProcessor into ProcessRowEndpoint.
          With that approach, we will need multiple CoprocessorProtocol. One for each implementation of RowProcessor.
          Because the implementation of the RowProcessor is now passed by the protocol class name instead of the Writable.

          For example, in our unit test, we have to do
          ---------------
          FriendsOfFriendsProtocol p =
          table.coprocessorProxy(FriendsOfFriendsProtocol.class, row);
          p.processRow();
          ---------------
          And the unit test needs to provide both FriendsOfFriendsProtocol and FriendsOfFriendsEndpoint.
          I feel this is not very convenient. I still prefer to pass the RowProcessor information via a Writable.

          The other thought is that the user of RowProcessor is actually HRegion.
          But ProcessRowEndpoint is used by the client via coprocessor.
          I feel that it would be nice to have them separated instead of listing their methods together.

          What do you think?

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

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

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Addressed Lars and Ted's comments @tedyu: I changed reference count to 31 and long count to 6. This is because we have added the following two things long rowProcessorTimeout RowProcessor.RowScanner rowScanner @lhofhansl: I have made most of the changes you suggested. Thanks! I tried to put RowProcessor into ProcessRowEndpoint. With that approach, we will need multiple CoprocessorProtocol. One for each implementation of RowProcessor. Because the implementation of the RowProcessor is now passed by the protocol class name instead of the Writable. For example, in our unit test, we have to do --------------- FriendsOfFriendsProtocol p = table.coprocessorProxy(FriendsOfFriendsProtocol.class, row); p.processRow(); --------------- And the unit test needs to provide both FriendsOfFriendsProtocol and FriendsOfFriendsEndpoint. I feel this is not very convenient. I still prefer to pass the RowProcessor information via a Writable. The other thought is that the user of RowProcessor is actually HRegion. But ProcessRowEndpoint is used by the client via coprocessor. I feel that it would be nice to have them separated instead of listing their methods together. What do you think? REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Thanks for the review, guys. I will update this soon.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:41 I see. Now I understand what you mean. Yes, there should really be only one class. I will make the change. Thanks!
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:44 That looks better. I will make the change.
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4791 Sorry for the mistake. I will fix it.

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Thanks for the review, guys. I will update this soon. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:41 I see. Now I understand what you mean. Yes, there should really be only one class. I will make the change. Thanks! src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:44 That looks better. I will make the change. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4791 Sorry for the mistake. I will fix it. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Fair enough on doing this in another jira.
          Looking at this jira, though, I realize you're almost there.

          The coprocessor endpoint itself can implement RowProcessor and then pass itself to HRegion.processRow. That way all class loading is done by the coprocessor framework.

          See inline comments below.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:44 Wanna have a RowProcessor interface and make this BaseRowProcessor (which of course would implement RowProcessor)?
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4372 Nice
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:54 Since this is for a single row (and hence for a single region) clients should just use the coprocessorProxy interface:
          RowProcessorProtocol p = table.coprocessorProxy(RowProcessorProtocol.class, myRow);
          p.process(myRowProcessorInstance);

          But see above, this method might not even be necessary.
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:41 Rather than having a method that requires another object passed (for which the class needs to be registered at all regionservers, etc). The endpoint itself could have the process method on it.
          A client would then only have to deploy the coprocessor (a different one for each type of operation, but that is less effort than a separate class for each operation, as coprocessors already implement the right class loading.)
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:44 I.e. here the coprocessor would pass itself.

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

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Fair enough on doing this in another jira. Looking at this jira, though, I realize you're almost there. The coprocessor endpoint itself can implement RowProcessor and then pass itself to HRegion.processRow. That way all class loading is done by the coprocessor framework. See inline comments below. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:44 Wanna have a RowProcessor interface and make this BaseRowProcessor (which of course would implement RowProcessor)? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4372 Nice src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:54 Since this is for a single row (and hence for a single region) clients should just use the coprocessorProxy interface: RowProcessorProtocol p = table.coprocessorProxy(RowProcessorProtocol.class, myRow); p.process(myRowProcessorInstance); But see above, this method might not even be necessary. src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:41 Rather than having a method that requires another object passed (for which the class needs to be registered at all regionservers, etc). The endpoint itself could have the process method on it. A client would then only have to deploy the coprocessor (a different one for each type of operation, but that is less effort than a separate class for each operation, as coprocessors already implement the right class loading.) src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java:44 I.e. here the coprocessor would pass itself. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4790 Please keep this at 30.
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4791 Please change this to 6, accounting for rowProcessorTimeout

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

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4790 Please keep this at 30. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4791 Please change this to 6, accounting for rowProcessorTimeout REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          @lhofhansl: I would prefer that to be the future work Thanks for the insights that you provide for the patch! That really helped a lot.

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". @lhofhansl: I would prefer that to be the future work Thanks for the insights that you provide for the patch! That really helped a lot. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Move completeMemstoreInsert() back to finally block.
          Ted: I think this should still be in finally block. Otherwise this may block all the reads.

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

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

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Move completeMemstoreInsert() back to finally block. Ted: I think this should still be in finally block. Otherwise this may block all the reads. REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          @sc:
          "It will be nice to have some API that can allows simple control on lock/mvcc. That will be very powerful."

          But you do not want to do it for this issue?

          My point is that a RowProcessor implementation class is what coprocessor endpoints were meant for (server side custom code) - with the caveat that coprocessors currently do not have enough access to regionserver details.

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

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". @sc: "It will be nice to have some API that can allows simple control on lock/mvcc. That will be very powerful." But you do not want to do it for this issue? My point is that a RowProcessor implementation class is what coprocessor endpoints were meant for (server side custom code) - with the caveat that coprocessors currently do not have enough access to regionserver details. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Fixed TestHeapSize

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

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

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Fixed TestHeapSize REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Inject some meta keyvalue in the unit test to demonstrate the use of walEdit

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

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

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Inject some meta keyvalue in the unit test to demonstrate the use of walEdit REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:72 By "internally", I mean for our internal use case.

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:72 By "internally", I mean for our internal use case. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Addressed Ted's last review comments

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

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

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Addressed Ted's last review comments REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Ted: Wow, that was super quick! Thanks!

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:72 This is actually for the application to override to create their own processor. I should document this more clearly. I will make the change.

          I am passing the walEdit here because internally we also submit some meta data to walEdit (kind of like you can send SQL comments to binlog). Basically passing walEdit here allow us to do more things.
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:43 good point.
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4379 yes. I will make the change.

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Ted: Wow, that was super quick! Thanks! INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:72 This is actually for the application to override to create their own processor. I should document this more clearly. I will make the change. I am passing the walEdit here because internally we also submit some meta data to walEdit (kind of like you can send SQL comments to binlog). Basically passing walEdit here allow us to do more things. src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:43 good point. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4379 yes. I will make the change. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          @lhofhansl: That's very interesting. HBASE-5529 is more general in the sense that It process multiple rows while this one process only one row but it allows more general process (read-modify-write). Maybe we can have a MultiRowProcessEndpoint in the future

          It will be nice to have some API that can allows simple control on lock/mvcc. That will be very powerful.

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". @lhofhansl: That's very interesting. HBASE-5529 is more general in the sense that It process multiple rows while this one process only one row but it allows more general process (read-modify-write). Maybe we can have a MultiRowProcessEndpoint in the future It will be nice to have some API that can allows simple control on lock/mvcc. That will be very powerful. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Good progress.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:43 Please explain type parameter T in javadoc.
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4379 mutations may have many elements.
          We should control the length of this log.
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:72 This method is used for testing.
          I suggest making it package private.

          Also, walEdits is empty upon entrance to this method.
          Do we need to expose it ? It is not used by friends of friends test.

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

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Good progress. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:43 Please explain type parameter T in javadoc. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4379 mutations may have many elements. We should control the length of this log. src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java:72 This method is used for testing. I suggest making it package private. Also, walEdits is empty upon entrance to this method. Do we need to expose it ? It is not used by friends of friends test. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Fixed a minor bug

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

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

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Fixed a minor bug REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Addressed Ted's review comments.
          Also 1. Remove unnecessary change in HRegionServer and HRegionInterface
          2. Make RowProcessor returns generic

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

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

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Addressed Ted's review comments. Also 1. Remove unnecessary change in HRegionServer and HRegionInterface 2. Make RowProcessor returns generic REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Here's another thought. This is actually slight different from HBASE-5229.

          HBASE-5229 provides one API for coprocessors to use, while this issue provides the code from the out side (the RowProcessor).

          I think it would be nice if there was an API on the RegionServer do execute something with lock/mvcc.
          Not sure about the actual API, but the coprocessor endpoint could be the RowProcessor, which would have the advantage that they could loaded dynamically and per table if needed.

          Maybe just public lockAndStartMvcc(row) and unlockAndCommitMvcc methods on RegionServer.
          Or executeAsTransaction(RowProcessor), or something.

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

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Here's another thought. This is actually slight different from HBASE-5229 . HBASE-5229 provides one API for coprocessors to use, while this issue provides the code from the out side (the RowProcessor). I think it would be nice if there was an API on the RegionServer do execute something with lock/mvcc. Not sure about the actual API, but the coprocessor endpoint could be the RowProcessor, which would have the advantage that they could loaded dynamically and per table if needed. Maybe just public lockAndStartMvcc(row) and unlockAndCommitMvcc methods on RegionServer. Or executeAsTransaction(RowProcessor), or something. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Ted: Thanks for the review! I will update this soon.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4364 I think you're right. If any of exception thrown, we actually don't want to completeMemstoreInsert().
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4368 goodcatch
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4318 good idea. I will make that change.

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Ted: Thanks for the review! I will update this soon. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4364 I think you're right. If any of exception thrown, we actually don't want to completeMemstoreInsert(). src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4368 goodcatch src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4318 good idea. I will make that change. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Thanks sc (Scott).
          This approach makes more sense to me. I'll review in more detail tomorrow.

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

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Thanks sc (Scott). This approach makes more sense to me. I'll review in more detail tomorrow. REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          INLINE COMMENTS
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java:210 Can we remove the braces for this code block (ending at line 223) ?
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4318 In the processor, multiple scans may be performed.
          Should we introduce timeout for this invocation ?
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4368 What if the wal sync was unsuccessful ?
          We should prepare to remove keys from memstore.
          See line 2204 of doMiniBatchPut().
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4364 Does step 9 have to be in the finally block ?

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

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". INLINE COMMENTS src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java:210 Can we remove the braces for this code block (ending at line 223) ? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4318 In the processor, multiple scans may be performed. Should we introduce timeout for this invocation ? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4368 What if the wal sync was unsuccessful ? We should prepare to remove keys from memstore. See line 2204 of doMiniBatchPut(). src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4364 Does step 9 have to be in the finally block ? REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Add a static helper function to allow easier use of the RowProcessor

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Add a static helper function to allow easier use of the RowProcessor REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Fixed some comments

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Fixed some comments REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Addressed Lar's review comments.
          Now the API is used from table.coprocessorExec().
          Thanks!

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java
          src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Addressed Lar's review comments. Now the API is used from table.coprocessorExec(). Thanks! REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowEndpoint.java src/main/java/org/apache/hadoop/hbase/coprocessor/ProcessRowProtocol.java src/main/java/org/apache/hadoop/hbase/coprocessor/RowProcessor.java src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/test/java/org/apache/hadoop/hbase/coprocessor/TestProcessRowEndpoint.java src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          @lhofhansl: After look at HBASE-5229, I think what you said makes sense. RowProcessor is some logic on server side and should belong to coprocessorExec() not a stand alone HTable API.

          I will make a change and update this patch soon. Thanks for pointing this out!

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". @lhofhansl: After look at HBASE-5229 , I think what you said makes sense. RowProcessor is some logic on server side and should belong to coprocessorExec() not a stand alone HTable API. I will make a change and update this patch soon. Thanks for pointing this out! REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Forgot to import InterfaceAudience

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/client/HTable.java
          src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          src/test/java/org/apache/hadoop/hbase/client/TestProcessRow.java
          src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Forgot to import InterfaceAudience REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/client/HTable.java src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java src/test/java/org/apache/hadoop/hbase/client/TestProcessRow.java src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          I will also read HBASE-5229 to see what are the alternatives. Thanks!

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". I will also read HBASE-5229 to see what are the alternatives. Thanks! REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Lars: Thanks for the feedback.

          Yes, the RowProcessor implementation needs to exist on the RegionServer. In our case, it's some predefined logic that we deployed on the server side.

          We also thought about using coprocessor also but like you said it will need to expose lots of details of HRegion. So it's not very easy.

          I think you have really good points about the API. We should be very careful about adding API because we can't take them back. The API should be as simple as possible.

          For our use case, this one is necessary because it allows execution of multiple commands on the server. This is very important for the performance. And I personally think the abstraction level is high enough to be a public API. What do you think?

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Lars: Thanks for the feedback. Yes, the RowProcessor implementation needs to exist on the RegionServer. In our case, it's some predefined logic that we deployed on the server side. We also thought about using coprocessor also but like you said it will need to expose lots of details of HRegion. So it's not very easy. I think you have really good points about the API. We should be very careful about adding API because we can't take them back. The API should be as simple as possible. For our use case, this one is necessary because it allows execution of multiple commands on the server. This is very important for the performance. And I personally think the abstraction level is high enough to be a public API. What do you think? REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          We're getting a lot of similar but slightly different APIs now.

          This is not a for a fixed use case, but basically this a command pattern where we pass an arbitrary command to the server execution there.

          The RowProcessor implementation needs to exist in the RegionServer's classpath, right?

          Like atomic multi row mutations, maybe this should only be accessible to a Coprocessor endpoint (see final resolution for HBASE-5229), and not to the public HTable API.

          Pie in the sky: It would be nice if the command itself could be implemented as a coprocessor endpoint (would need to expose locks and mvcc to region-coprocessors, so maybe that's overkill).

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

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". We're getting a lot of similar but slightly different APIs now. This is not a for a fixed use case, but basically this a command pattern where we pass an arbitrary command to the server execution there. The RowProcessor implementation needs to exist in the RegionServer's classpath, right? Like atomic multi row mutations, maybe this should only be accessible to a Coprocessor endpoint (see final resolution for HBASE-5229 ), and not to the public HTable API. Pie in the sky: It would be nice if the command itself could be implemented as a coprocessor endpoint (would need to expose locks and mvcc to region-coprocessors, so maybe that's overkill). REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Ted: Yes, this API allows read-modify-write atomically. Also you can perform multiple reads with dependency on previous results (for example, querying friends of friends in the unit test).

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Ted: Yes, this API allows read-modify-write atomically. Also you can perform multiple reads with dependency on previous results (for example, querying friends of friends in the unit test). REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Addressed Ted's comments

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/client/HTable.java
          src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          src/test/java/org/apache/hadoop/hbase/client/TestProcessRow.java
          src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Addressed Ted's comments REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/client/HTable.java src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java src/test/java/org/apache/hadoop/hbase/client/TestProcessRow.java src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java
          Hide
          Phabricator added a comment -

          sc has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Ted: Thanks for the review. I will update this soon.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java:268 Thanks! good catch
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4376 Good catch!

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

          Show
          Phabricator added a comment - sc has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Ted: Thanks for the review. I will update this soon. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java:268 Thanks! good catch src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4376 Good catch! REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java:31 Please tag this interface @InterfaceAudience.Public

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

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java:31 Please tag this interface @InterfaceAudience.Public REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          Only small tests were run in https://builds.apache.org/job/PreCommit-HBASE-Build/1085

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java:28 Discard previous comment.
          The scanner can be used to perform multiple scans.
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java:268 Please add an assertion at the end of TestHbaseObjectWritable.testGetClassCode() for RowProcessor.

          Change the ordinal to 83 in TestHbaseObjectWritable.testGetNextObjectCode()
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4376 1 ClassSize.REFERENCE should be added to HRegion.FIXED_OVERHEAD

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

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Only small tests were run in https://builds.apache.org/job/PreCommit-HBASE-Build/1085 INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java:28 Discard previous comment. The scanner can be used to perform multiple scans. src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java:268 Please add an assertion at the end of TestHbaseObjectWritable.testGetClassCode() for RowProcessor. Change the ordinal to 83 in TestHbaseObjectWritable.testGetNextObjectCode() src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:4376 1 ClassSize.REFERENCE should be added to HRegion.FIXED_OVERHEAD REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".

          The difference between this API and mutateRow() is that RowMutations don't cover scan, right ?

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java:28 There is only one RowProcessor.RowScanner passed to process().
          So scan should be singular.
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java:268 I think this should be moved to after GENERIC_ARRAY_CODE, line 272

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

          Show
          Phabricator added a comment - tedyu has commented on the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". The difference between this API and mutateRow() is that RowMutations don't cover scan, right ? INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java:28 There is only one RowProcessor.RowScanner passed to process(). So scan should be singular. src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java:268 I think this should be moved to after GENERIC_ARRAY_CODE, line 272 REVISION DETAIL https://reviews.facebook.net/D2067
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          More comments change

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/client/HTable.java
          src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          src/test/java/org/apache/hadoop/hbase/client/TestProcessRow.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA More comments change REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/client/HTable.java src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java src/test/java/org/apache/hadoop/hbase/client/TestProcessRow.java
          Hide
          Phabricator added a comment -

          sc updated the revision "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Update some comments

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/client/HTable.java
          src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          src/test/java/org/apache/hadoop/hbase/client/TestProcessRow.java

          Show
          Phabricator added a comment - sc updated the revision " HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Update some comments REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/client/HTable.java src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java src/test/java/org/apache/hadoop/hbase/client/TestProcessRow.java
          Hide
          Phabricator added a comment -

          sc requested code review of "HBASE-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row".
          Reviewers: tedyu, dhruba, JIRA

          Add a read-modify-write row operation

          Task ID: #

          Blame Rev:

          We have modified HRegion.java internally to do some atomic row processing. It will be nice to have a plugable API for this.

          TEST PLAN
          unit test

          Revert Plan:

          Tags:

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/client/HTable.java
          src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java
          src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
          src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          src/test/java/org/apache/hadoop/hbase/client/TestProcessRow.java

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

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

          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-5515 [jira] Add a processRow API that supports atomic multiple reads and writes on a row". Reviewers: tedyu, dhruba, JIRA Add a read-modify-write row operation Task ID: # Blame Rev: We have modified HRegion.java internally to do some atomic row processing. It will be nice to have a plugable API for this. TEST PLAN unit test Revert Plan: Tags: REVISION DETAIL https://reviews.facebook.net/D2067 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/client/HTable.java src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java src/main/java/org/apache/hadoop/hbase/client/RowProcessor.java src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java src/test/java/org/apache/hadoop/hbase/client/TestProcessRow.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/4425/ 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