Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-3584 Allow atomic put/delete in one call
  3. HBASE-5203

Group atomic put/delete operation into a single WALEdit to handle region server failures.

    Details

    • Hadoop Flags:
      Reviewed

      Description

      HBASE-3584 does not not provide fully atomic operation in case of region server failures (see explanation there).

      What should happen is that either (1) all edits are applied via a single WALEdit, or (2) the WALEdits are applied in async mode and then sync'ed together.

      For #1 it is not clear whether it is advisable to manage multiple different operations (Put/Delete) via a single WAL edit. A quick check reveals that WAL replay on region startup would work, but that replication would need to be adapted. The refactoring needed would be non-trivial.

      #2 Might actually not work, as another operation could request sync'ing a later edit and hence flush these entries out as well.

      Addendum:
      The attached patch implements #1 and fixes replication to be able to deal with different operations being grouped in one WALEdit.

      1. 5203.txt
        23 kB
        Lars Hofhansl
      2. 5203-v3.txt
        26 kB
        Lars Hofhansl

        Activity

        Hide
        zhihyu@ebaysf.com Ted Yu added a comment - - edited

        I think we should log a JIRA for adapting replication to work with option #1.
        After that JIRA is completed, we can revisit this JIRA.

        Show
        zhihyu@ebaysf.com Ted Yu added a comment - - edited I think we should log a JIRA for adapting replication to work with option #1. After that JIRA is completed, we can revisit this JIRA.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Agreed. #1 might actually be tricky if there are multiple Deletes in a row, because it is not possible to tell from KVs in the WALEdit to tell when one Delete (or Put) ends and the next one starts.

        For Puts this does not matter as the KVs can all just be added to one Put operation. For Deletes, however, that is not possible. But it should be possible for Deletes to group all version-deletes and column-deletes together and handle family-deletes separately.

        I think #1 is the only correct option.

        Show
        lhofhansl Lars Hofhansl added a comment - Agreed. #1 might actually be tricky if there are multiple Deletes in a row, because it is not possible to tell from KVs in the WALEdit to tell when one Delete (or Put) ends and the next one starts. For Puts this does not matter as the KVs can all just be added to one Put operation. For Deletes, however, that is not possible. But it should be possible for Deletes to group all version-deletes and column-deletes together and handle family-deletes separately. I think #1 is the only correct option.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        The refactoring is in the Put/Delete code, not in the replication code, so I don't think it needs another jira, actually.

        Show
        lhofhansl Lars Hofhansl added a comment - The refactoring is in the Put/Delete code, not in the replication code, so I don't think it needs another jira, actually.
        Hide
        zhihyu@ebaysf.com Ted Yu added a comment -

        I think we should collect use case requirement from users.
        For example, why would the same operation delete a family and at the same time update some columns in another family ?

        Show
        zhihyu@ebaysf.com Ted Yu added a comment - I think we should collect use case requirement from users. For example, why would the same operation delete a family and at the same time update some columns in another family ?
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Because the Delete might come before the Put or vice versa? Put and Delete are not commutative, their order matters.
        I think that is reasonable. Just like from the client you can issue Deletes and Puts in any order for the same family.

        But... Looking at Delete.java, there is some other weirdness there:
        When you add a family marker for a family it deletes all column/version markers for that family, but does not check the timestamp. When can't I issue a Delete that deletes a family F for time T1 and some columns of that family for time T2 (when T2 > T1)?
        Note that the same it true for column makers that hide version markers... In theory we could remove the version markers too.

        If that strange logic would not exist in Delete, this would become simpler. (It would incidentally also simplify the Import/Copytable MR code).

        So the first thing I propose is removing that logic in Delete.java. You can just add family/column/version markers to a Delete as you please. If we want to be fancy we can track the latest TS for columnfamily and then remove all entries if superseded by a family delete marker - but I do not even think that is necessary.

        Show
        lhofhansl Lars Hofhansl added a comment - Because the Delete might come before the Put or vice versa? Put and Delete are not commutative, their order matters. I think that is reasonable. Just like from the client you can issue Deletes and Puts in any order for the same family. But... Looking at Delete.java, there is some other weirdness there: When you add a family marker for a family it deletes all column/version markers for that family, but does not check the timestamp. When can't I issue a Delete that deletes a family F for time T1 and some columns of that family for time T2 (when T2 > T1)? Note that the same it true for column makers that hide version markers... In theory we could remove the version markers too. If that strange logic would not exist in Delete, this would become simpler. (It would incidentally also simplify the Import/Copytable MR code). So the first thing I propose is removing that logic in Delete.java. You can just add family/column/version markers to a Delete as you please. If we want to be fancy we can track the latest TS for columnfamily and then remove all entries if superseded by a family delete marker - but I do not even think that is necessary.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Filed HBASE-5205 for the Delete issue.

        For this issue here I will change the Delete(KV) constructor that I added with HBASE-4682 to a deleteKV(KeyValue) method (which is what I should have done in HBASE-4682).

        Show
        lhofhansl Lars Hofhansl added a comment - Filed HBASE-5205 for the Delete issue. For this issue here I will change the Delete(KV) constructor that I added with HBASE-4682 to a deleteKV(KeyValue) method (which is what I should have done in HBASE-4682 ).
        Hide
        lhofhansl Lars Hofhansl added a comment -

        First version of a patch.
        Passes all my local tests.

        Unfortunately it is almost a complete rewrite of HBASE-3584.

        Will also upload to RB.

        Show
        lhofhansl Lars Hofhansl added a comment - First version of a patch. Passes all my local tests. Unfortunately it is almost a complete rewrite of HBASE-3584 . Will also upload to RB.
        Hide
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3510/
        -----------------------------------------------------------

        Review request for hbase, Ted Yu and Michael Stack.

        Summary
        -------

        Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call".
        This makes this actually correct in the case of RegionServer failures (HBASE-3485 was correct for all scenarios but RegionServer failures).
        HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied.
        This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that.
        WAL recovery already handles this case.

        This addresses bug HBASE-5203.
        https://issues.apache.org/jira/browse/HBASE-5203

        Diffs


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744

        Diff: https://reviews.apache.org/r/3510/diff

        Testing
        -------

        • Tests added in HBASE-3485
        • manual testing.
        • getting a full test run right now

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/ ----------------------------------------------------------- Review request for hbase, Ted Yu and Michael Stack. Summary ------- Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call". This makes this actually correct in the case of RegionServer failures ( HBASE-3485 was correct for all scenarios but RegionServer failures). HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied. This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that. WAL recovery already handles this case. This addresses bug HBASE-5203 . https://issues.apache.org/jira/browse/HBASE-5203 Diffs http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744 Diff: https://reviews.apache.org/r/3510/diff Testing ------- Tests added in HBASE-3485 manual testing. getting a full test run right now Thanks, Lars
        Hide
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3510/
        -----------------------------------------------------------

        (Updated 2012-01-16 07:58:33.454515)

        Review request for hbase, Ted Yu and Michael Stack.

        Changes
        -------

        Fixed comments (before Ted points them out to me )

        Summary
        -------

        Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call".
        This makes this actually correct in the case of RegionServer failures (HBASE-3485 was correct for all scenarios but RegionServer failures).
        HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied.
        This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that.
        WAL recovery already handles this case.

        This addresses bug HBASE-5203.
        https://issues.apache.org/jira/browse/HBASE-5203

        Diffs (updated)


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744

        Diff: https://reviews.apache.org/r/3510/diff

        Testing
        -------

        • Tests added in HBASE-3485
        • manual testing.
        • getting a full test run right now

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/ ----------------------------------------------------------- (Updated 2012-01-16 07:58:33.454515) Review request for hbase, Ted Yu and Michael Stack. Changes ------- Fixed comments (before Ted points them out to me ) Summary ------- Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call". This makes this actually correct in the case of RegionServer failures ( HBASE-3485 was correct for all scenarios but RegionServer failures). HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied. This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that. WAL recovery already handles this case. This addresses bug HBASE-5203 . https://issues.apache.org/jira/browse/HBASE-5203 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744 Diff: https://reviews.apache.org/r/3510/diff Testing ------- Tests added in HBASE-3485 manual testing. getting a full test run right now Thanks, Lars
        Hide
        hadoopqa Hadoop QA added a comment -

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

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

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

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

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

        -1 findbugs. The patch appears to introduce 82 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.master.TestDistributedLogSplitting
        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/774//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/774//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/774//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12510675/5203.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -145 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 82 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.master.TestDistributedLogSplitting 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/774//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/774//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/774//console This message is automatically generated.
        Hide
        ram_krish ramkrishna.s.vasudevan added a comment -

        @Lars
        The coprocessor postPut and postDelete

        if (m instanceof Put) {
                      coprocessorHost.postPut((Put) m, walEdits.get(i),
                          m.getWriteToWAL());
                    } else if (m instanceof Delete) {
                      coprocessorHost.postDelete((Delete) m, walEdits.get(i),
                          m.getWriteToWAL());
                    }
        

        Can this be done even if any failures in the internalPut or internalDelete()? Just correct me if am wrong.

        Show
        ram_krish ramkrishna.s.vasudevan added a comment - @Lars The coprocessor postPut and postDelete if (m instanceof Put) { coprocessorHost.postPut((Put) m, walEdits.get(i), m.getWriteToWAL()); } else if (m instanceof Delete) { coprocessorHost.postDelete((Delete) m, walEdits.get(i), m.getWriteToWAL()); } Can this be done even if any failures in the internalPut or internalDelete()? Just correct me if am wrong.
        Hide
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3510/#review4394
        -----------------------------------------------------------

        Nice work, Lars.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java
        <https://reviews.apache.org/r/3510/#comment9902>

        I think DoNotRetryIOException may be more appropriate here.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/3510/#comment9897>

        White space.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/3510/#comment9898>

        Please replace this parameter with clusterId.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/3510/#comment9899>

        Please add clusterId parameter here.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/3510/#comment9903>

        Should we allow caller to pass clusterId ?
        That parameter would be used at line 4213.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/3510/#comment9900>

        The original intent of this check being inside for loop was to populate walEdits.
        Now we can lift this check to after line 4157.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/3510/#comment9901>

        There is only one WALEdit now, right ?

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
        <https://reviews.apache.org/r/3510/#comment9904>

        I think the original javadoc should be modified to indicate the support of Put and Delete.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
        <https://reviews.apache.org/r/3510/#comment9906>

        Good.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
        <https://reviews.apache.org/r/3510/#comment9907>

        I think 'to a map from key to values' may be clearer.
        Otherwise people have to read the method body to fully understand.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
        <https://reviews.apache.org/r/3510/#comment9908>

        I don't see InterruptedException declared to be thrown by this method.
        IE is caught at line 171.

        • Ted

        On 2012-01-16 07:58:33, Lars Hofhansl wrote:

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

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/3510/

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

        (Updated 2012-01-16 07:58:33)

        Review request for hbase, Ted Yu and Michael Stack.

        Summary

        -------

        Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call".

        This makes this actually correct in the case of RegionServer failures (HBASE-3485 was correct for all scenarios but RegionServer failures).

        HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied.

        This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that.

        WAL recovery already handles this case.

        This addresses bug HBASE-5203.

        https://issues.apache.org/jira/browse/HBASE-5203

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744

        Diff: https://reviews.apache.org/r/3510/diff

        Testing

        -------

        * Tests added in HBASE-3485

        * manual testing.

        * getting a full test run right now

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/#review4394 ----------------------------------------------------------- Nice work, Lars. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java < https://reviews.apache.org/r/3510/#comment9902 > I think DoNotRetryIOException may be more appropriate here. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/3510/#comment9897 > White space. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/3510/#comment9898 > Please replace this parameter with clusterId. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/3510/#comment9899 > Please add clusterId parameter here. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/3510/#comment9903 > Should we allow caller to pass clusterId ? That parameter would be used at line 4213. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/3510/#comment9900 > The original intent of this check being inside for loop was to populate walEdits. Now we can lift this check to after line 4157. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/3510/#comment9901 > There is only one WALEdit now, right ? http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java < https://reviews.apache.org/r/3510/#comment9904 > I think the original javadoc should be modified to indicate the support of Put and Delete. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java < https://reviews.apache.org/r/3510/#comment9906 > Good. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java < https://reviews.apache.org/r/3510/#comment9907 > I think 'to a map from key to values' may be clearer. Otherwise people have to read the method body to fully understand. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java < https://reviews.apache.org/r/3510/#comment9908 > I don't see InterruptedException declared to be thrown by this method. IE is caught at line 171. Ted On 2012-01-16 07:58:33, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/ ----------------------------------------------------------- (Updated 2012-01-16 07:58:33) Review request for hbase, Ted Yu and Michael Stack. Summary ------- Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call". This makes this actually correct in the case of RegionServer failures ( HBASE-3485 was correct for all scenarios but RegionServer failures). HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied. This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that. WAL recovery already handles this case. This addresses bug HBASE-5203 . https://issues.apache.org/jira/browse/HBASE-5203 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744 Diff: https://reviews.apache.org/r/3510/diff Testing ------- * Tests added in HBASE-3485 * manual testing. * getting a full test run right now Thanks, Lars
        Hide
        lhofhansl Lars Hofhansl added a comment -

        @Ram: are you looking at the right patch? The part you quote was removed with this.
        Or are you saying it should be possible to do that?

        I think that would not correct, as I want an atomic operation and I realized in HBASE-3584 that I need to write a single WALEdit.

        Show
        lhofhansl Lars Hofhansl added a comment - @Ram: are you looking at the right patch? The part you quote was removed with this. Or are you saying it should be possible to do that? I think that would not correct, as I want an atomic operation and I realized in HBASE-3584 that I need to write a single WALEdit.
        Hide
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

        On 2012-01-16 15:27:14, Ted Yu wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java, line 149

        > <https://reviews.apache.org/r/3510/diff/2/?file=68986#file68986line149>

        >

        > I think DoNotRetryIOException may be more appropriate here.

        Sure. Although this is client side code, so there is no notion of retry. (Put does the same)

        On 2012-01-16 15:27:14, Ted Yu wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 4170

        > <https://reviews.apache.org/r/3510/diff/2/?file=68987#file68987line4170>

        >

        > Should we allow caller to pass clusterId ?

        > That parameter would be used at line 4213.

        The clusterID is only used for replication. Only plain Puts and Deletes need to use an optional clusterId (when executed from the ReplicationSink). All other operations do (and should) use the local clusterID.

        On 2012-01-16 15:27:14, Ted Yu wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 4184

        > <https://reviews.apache.org/r/3510/diff/2/?file=68987#file68987line4184>

        >

        > The original intent of this check being inside for loop was to populate walEdits.

        > Now we can lift this check to after line 4157.

        Correct. But I still need to execute and check all preHooks before the 1st WALEdit is written.

        On 2012-01-16 15:27:14, Ted Yu wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 4214

        > <https://reviews.apache.org/r/3510/diff/2/?file=68987#file68987line4214>

        >

        > There is only one WALEdit now, right ?

        Correct. Should read "and apply edits" (there are many edits in the one WALEdit)

        On 2012-01-16 15:27:14, Ted Yu wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java, line 98

        > <https://reviews.apache.org/r/3510/diff/2/?file=68988#file68988line98>

        >

        > I think the original javadoc should be modified to indicate the support of Put and Delete.

        Agreed.

        On 2012-01-16 15:27:14, Ted Yu wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java, line 175

        > <https://reviews.apache.org/r/3510/diff/2/?file=68988#file68988line175>

        >

        > I think 'to a map from key to values' may be clearer.

        > Otherwise people have to read the method body to fully understand.

        I think this should be a static util method somewhere

        On 2012-01-16 15:27:14, Ted Yu wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java, line 202

        > <https://reviews.apache.org/r/3510/diff/2/?file=68988#file68988line202>

        >

        > I don't see InterruptedException declared to be thrown by this method.

        > IE is caught at line 171.

        Argghh... HTable.batch() throws it, and my first attempt was to pass it on. This is a leftover will be remove.
        Thanks for the keen eyes.

        On 2012-01-16 15:27:14, Ted Yu wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1788

        > <https://reviews.apache.org/r/3510/diff/2/?file=68987#file68987line1788>

        >

        > Please replace this parameter with clusterId.

        I knew you would find some Javadoc I missed
        Will fix.

        • Lars

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3510/#review4394
        -----------------------------------------------------------

        On 2012-01-16 07:58:33, Lars Hofhansl wrote:

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

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/3510/

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

        (Updated 2012-01-16 07:58:33)

        Review request for hbase, Ted Yu and Michael Stack.

        Summary

        -------

        Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call".

        This makes this actually correct in the case of RegionServer failures (HBASE-3485 was correct for all scenarios but RegionServer failures).

        HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied.

        This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that.

        WAL recovery already handles this case.

        This addresses bug HBASE-5203.

        https://issues.apache.org/jira/browse/HBASE-5203

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744

        Diff: https://reviews.apache.org/r/3510/diff

        Testing

        -------

        * Tests added in HBASE-3485

        * manual testing.

        * getting a full test run right now

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - On 2012-01-16 15:27:14, Ted Yu wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java , line 149 > < https://reviews.apache.org/r/3510/diff/2/?file=68986#file68986line149 > > > I think DoNotRetryIOException may be more appropriate here. Sure. Although this is client side code, so there is no notion of retry. (Put does the same) On 2012-01-16 15:27:14, Ted Yu wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java , line 4170 > < https://reviews.apache.org/r/3510/diff/2/?file=68987#file68987line4170 > > > Should we allow caller to pass clusterId ? > That parameter would be used at line 4213. The clusterID is only used for replication. Only plain Puts and Deletes need to use an optional clusterId (when executed from the ReplicationSink). All other operations do (and should) use the local clusterID. On 2012-01-16 15:27:14, Ted Yu wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java , line 4184 > < https://reviews.apache.org/r/3510/diff/2/?file=68987#file68987line4184 > > > The original intent of this check being inside for loop was to populate walEdits. > Now we can lift this check to after line 4157. Correct. But I still need to execute and check all preHooks before the 1st WALEdit is written. On 2012-01-16 15:27:14, Ted Yu wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java , line 4214 > < https://reviews.apache.org/r/3510/diff/2/?file=68987#file68987line4214 > > > There is only one WALEdit now, right ? Correct. Should read "and apply edits" (there are many edits in the one WALEdit) On 2012-01-16 15:27:14, Ted Yu wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java , line 98 > < https://reviews.apache.org/r/3510/diff/2/?file=68988#file68988line98 > > > I think the original javadoc should be modified to indicate the support of Put and Delete. Agreed. On 2012-01-16 15:27:14, Ted Yu wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java , line 175 > < https://reviews.apache.org/r/3510/diff/2/?file=68988#file68988line175 > > > I think 'to a map from key to values' may be clearer. > Otherwise people have to read the method body to fully understand. I think this should be a static util method somewhere On 2012-01-16 15:27:14, Ted Yu wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java , line 202 > < https://reviews.apache.org/r/3510/diff/2/?file=68988#file68988line202 > > > I don't see InterruptedException declared to be thrown by this method. > IE is caught at line 171. Argghh... HTable.batch() throws it, and my first attempt was to pass it on. This is a leftover will be remove. Thanks for the keen eyes. On 2012-01-16 15:27:14, Ted Yu wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java , line 1788 > < https://reviews.apache.org/r/3510/diff/2/?file=68987#file68987line1788 > > > Please replace this parameter with clusterId. I knew you would find some Javadoc I missed Will fix. Lars ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/#review4394 ----------------------------------------------------------- On 2012-01-16 07:58:33, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/ ----------------------------------------------------------- (Updated 2012-01-16 07:58:33) Review request for hbase, Ted Yu and Michael Stack. Summary ------- Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call". This makes this actually correct in the case of RegionServer failures ( HBASE-3485 was correct for all scenarios but RegionServer failures). HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied. This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that. WAL recovery already handles this case. This addresses bug HBASE-5203 . https://issues.apache.org/jira/browse/HBASE-5203 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744 Diff: https://reviews.apache.org/r/3510/diff Testing ------- * Tests added in HBASE-3485 * manual testing. * getting a full test run right now Thanks, Lars
        Hide
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3510/#review4397
        -----------------------------------------------------------

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/3510/#comment9917>

        What I meant was that if coprocessorHost == null, the for loop can be skipped.

        • Ted

        On 2012-01-16 07:58:33, Lars Hofhansl wrote:

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

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/3510/

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

        (Updated 2012-01-16 07:58:33)

        Review request for hbase, Ted Yu and Michael Stack.

        Summary

        -------

        Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call".

        This makes this actually correct in the case of RegionServer failures (HBASE-3485 was correct for all scenarios but RegionServer failures).

        HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied.

        This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that.

        WAL recovery already handles this case.

        This addresses bug HBASE-5203.

        https://issues.apache.org/jira/browse/HBASE-5203

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744

        Diff: https://reviews.apache.org/r/3510/diff

        Testing

        -------

        * Tests added in HBASE-3485

        * manual testing.

        * getting a full test run right now

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/#review4397 ----------------------------------------------------------- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/3510/#comment9917 > What I meant was that if coprocessorHost == null, the for loop can be skipped. Ted On 2012-01-16 07:58:33, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/ ----------------------------------------------------------- (Updated 2012-01-16 07:58:33) Review request for hbase, Ted Yu and Michael Stack. Summary ------- Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call". This makes this actually correct in the case of RegionServer failures ( HBASE-3485 was correct for all scenarios but RegionServer failures). HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied. This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that. WAL recovery already handles this case. This addresses bug HBASE-5203 . https://issues.apache.org/jira/browse/HBASE-5203 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744 Diff: https://reviews.apache.org/r/3510/diff Testing ------- * Tests added in HBASE-3485 * manual testing. * getting a full test run right now Thanks, Lars
        Hide
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

        On 2012-01-16 16:25:31, Ted Yu wrote:

        > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 4184

        > <https://reviews.apache.org/r/3510/diff/2/?file=68987#file68987line4184>

        >

        > What I meant was that if coprocessorHost == null, the for loop can be skipped.

        Oh I see. You're right.

        • Lars

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3510/#review4397
        -----------------------------------------------------------

        On 2012-01-16 07:58:33, Lars Hofhansl wrote:

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

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/3510/

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

        (Updated 2012-01-16 07:58:33)

        Review request for hbase, Ted Yu and Michael Stack.

        Summary

        -------

        Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call".

        This makes this actually correct in the case of RegionServer failures (HBASE-3485 was correct for all scenarios but RegionServer failures).

        HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied.

        This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that.

        WAL recovery already handles this case.

        This addresses bug HBASE-5203.

        https://issues.apache.org/jira/browse/HBASE-5203

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744

        Diff: https://reviews.apache.org/r/3510/diff

        Testing

        -------

        * Tests added in HBASE-3485

        * manual testing.

        * getting a full test run right now

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - On 2012-01-16 16:25:31, Ted Yu wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java , line 4184 > < https://reviews.apache.org/r/3510/diff/2/?file=68987#file68987line4184 > > > What I meant was that if coprocessorHost == null, the for loop can be skipped. Oh I see. You're right. Lars ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/#review4397 ----------------------------------------------------------- On 2012-01-16 07:58:33, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/ ----------------------------------------------------------- (Updated 2012-01-16 07:58:33) Review request for hbase, Ted Yu and Michael Stack. Summary ------- Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call". This makes this actually correct in the case of RegionServer failures ( HBASE-3485 was correct for all scenarios but RegionServer failures). HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied. This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that. WAL recovery already handles this case. This addresses bug HBASE-5203 . https://issues.apache.org/jira/browse/HBASE-5203 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744 Diff: https://reviews.apache.org/r/3510/diff Testing ------- * Tests added in HBASE-3485 * manual testing. * getting a full test run right now Thanks, Lars
        Hide
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3510/
        -----------------------------------------------------------

        (Updated 2012-01-16 17:28:09.639619)

        Review request for hbase, Ted Yu and Michael Stack.

        Changes
        -------

        • Addresses Ted's comments.
        • Passes all tests.

        Summary
        -------

        Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call".
        This makes this actually correct in the case of RegionServer failures (HBASE-3485 was correct for all scenarios but RegionServer failures).
        HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied.
        This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that.
        WAL recovery already handles this case.

        This addresses bug HBASE-5203.
        https://issues.apache.org/jira/browse/HBASE-5203

        Diffs (updated)


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231744

        Diff: https://reviews.apache.org/r/3510/diff

        Testing
        -------

        • Tests added in HBASE-3485
        • manual testing.
        • getting a full test run right now

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/ ----------------------------------------------------------- (Updated 2012-01-16 17:28:09.639619) Review request for hbase, Ted Yu and Michael Stack. Changes ------- Addresses Ted's comments. Passes all tests. Summary ------- Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call". This makes this actually correct in the case of RegionServer failures ( HBASE-3485 was correct for all scenarios but RegionServer failures). HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied. This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that. WAL recovery already handles this case. This addresses bug HBASE-5203 . https://issues.apache.org/jira/browse/HBASE-5203 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231744 Diff: https://reviews.apache.org/r/3510/diff Testing ------- Tests added in HBASE-3485 manual testing. getting a full test run right now Thanks, Lars
        Hide
        ram_krish ramkrishna.s.vasudevan added a comment -

        @Lars
        Sorry i pasted the snippet from the code. If you take doMiniBatchPuts the postPut() will be done onlly if the put is successful.
        Here in mutateRow() we do the postPut in finally block.
        So just i wanted to know if we the MutatedRow's log.append() fails we still execute the postPut(). Pls do correct me if am wrong. I get the intent behind the patch but this part am not sure.

        Show
        ram_krish ramkrishna.s.vasudevan added a comment - @Lars Sorry i pasted the snippet from the code. If you take doMiniBatchPuts the postPut() will be done onlly if the put is successful. Here in mutateRow() we do the postPut in finally block. So just i wanted to know if we the MutatedRow's log.append() fails we still execute the postPut(). Pls do correct me if am wrong. I get the intent behind the patch but this part am not sure.
        Hide
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3510/#review4400
        -----------------------------------------------------------

        Ship it!

        • Ted

        On 2012-01-16 17:28:09, Lars Hofhansl wrote:

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

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/3510/

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

        (Updated 2012-01-16 17:28:09)

        Review request for hbase, Ted Yu and Michael Stack.

        Summary

        -------

        Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call".

        This makes this actually correct in the case of RegionServer failures (HBASE-3485 was correct for all scenarios but RegionServer failures).

        HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied.

        This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that.

        WAL recovery already handles this case.

        This addresses bug HBASE-5203.

        https://issues.apache.org/jira/browse/HBASE-5203

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231744

        Diff: https://reviews.apache.org/r/3510/diff

        Testing

        -------

        * Tests added in HBASE-3485

        * manual testing.

        * getting a full test run right now

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/#review4400 ----------------------------------------------------------- Ship it! Ted On 2012-01-16 17:28:09, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/ ----------------------------------------------------------- (Updated 2012-01-16 17:28:09) Review request for hbase, Ted Yu and Michael Stack. Summary ------- Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call". This makes this actually correct in the case of RegionServer failures ( HBASE-3485 was correct for all scenarios but RegionServer failures). HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied. This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that. WAL recovery already handles this case. This addresses bug HBASE-5203 . https://issues.apache.org/jira/browse/HBASE-5203 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231744 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231744 Diff: https://reviews.apache.org/r/3510/diff Testing ------- * Tests added in HBASE-3485 * manual testing. * getting a full test run right now Thanks, Lars
        Hide
        lhofhansl Lars Hofhansl added a comment -

        @Ram: I see what you mean. Good point.
        Unlike doMiniBatchPut there is no partial completion here, but the postHooks should indeed only be run if the (entire) operation was successful. I'll have a change soon.

        Show
        lhofhansl Lars Hofhansl added a comment - @Ram: I see what you mean. Good point. Unlike doMiniBatchPut there is no partial completion here, but the postHooks should indeed only be run if the (entire) operation was successful. I'll have a change soon.
        Hide
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3510/
        -----------------------------------------------------------

        (Updated 2012-01-16 19:05:01.756466)

        Review request for hbase, Ted Yu and Michael Stack.

        Changes
        -------

        • Addressing Ram's comments. Coprocessor postHooks are now only executed if all operations were successful (threw no exceptions).

        Summary
        -------

        Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call".
        This makes this actually correct in the case of RegionServer failures (HBASE-3485 was correct for all scenarios but RegionServer failures).
        HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied.
        This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that.
        WAL recovery already handles this case.

        This addresses bug HBASE-5203.
        https://issues.apache.org/jira/browse/HBASE-5203

        Diffs (updated)


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1232110
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1232110
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1232110
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1232110
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1232110

        Diff: https://reviews.apache.org/r/3510/diff

        Testing (updated)
        -------

        • Tests added in HBASE-3485
        • manual testing.
        • passes all tests.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3510/ ----------------------------------------------------------- (Updated 2012-01-16 19:05:01.756466) Review request for hbase, Ted Yu and Michael Stack. Changes ------- Addressing Ram's comments. Coprocessor postHooks are now only executed if all operations were successful (threw no exceptions). Summary ------- Basically a rewrite (sorry about that) of HBASE-3485 "Allow atomic put/delete in one call". This makes this actually correct in the case of RegionServer failures ( HBASE-3485 was correct for all scenarios but RegionServer failures). HRegion.mutateRow(...) now groups all edits into a single WALEdit and appends all edits in one call. Only then are the memstore edits applied. This is the first time that WALEdits can contain KVs from different types of operations. So I also had to fix the replication code to understand that. WAL recovery already handles this case. This addresses bug HBASE-5203 . https://issues.apache.org/jira/browse/HBASE-5203 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java 1232110 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1232110 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java 1232110 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1232110 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1232110 Diff: https://reviews.apache.org/r/3510/diff Testing (updated) ------- Tests added in HBASE-3485 manual testing. passes all tests. Thanks, Lars
        Hide
        lhofhansl Lars Hofhansl added a comment -

        @Ram: added a new patch to RB. Only change is wrapping the whole operation in try/finally and running the postHooks outside the finally blocks (but still after the mvcc is rolled forward and the regionlock was released). Otherwise the patch is identical.
        Please have a look. Thanks.

        Show
        lhofhansl Lars Hofhansl added a comment - @Ram: added a new patch to RB. Only change is wrapping the whole operation in try/finally and running the postHooks outside the finally blocks (but still after the mvcc is rolled forward and the regionlock was released). Otherwise the patch is identical. Please have a look. Thanks.
        Hide
        ram_krish ramkrishna.s.vasudevan added a comment -

        +1.
        Thanks Lars.

        Show
        ram_krish ramkrishna.s.vasudevan added a comment - +1. Thanks Lars.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Ok... Will commit later today.
        @Stack: Wanna have a quick look?

        Show
        lhofhansl Lars Hofhansl added a comment - Ok... Will commit later today. @Stack: Wanna have a quick look?
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Double checking latest patch (same as on RB)

        Show
        lhofhansl Lars Hofhansl added a comment - Double checking latest patch (same as on RB)
        Hide
        hadoopqa Hadoop QA added a comment -

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

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

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

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

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

        -1 findbugs. The patch appears to introduce 82 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/781//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/781//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/781//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12510770/5203-v3.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -145 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 82 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/781//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/781//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/781//console This message is automatically generated.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Committed to trunk

        Show
        lhofhansl Lars Hofhansl added a comment - Committed to trunk
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK-security #81 (See https://builds.apache.org/job/HBase-TRUNK-security/81/)
        HBASE-5203 Group atomic put/delete operation into a single WALEdit to handle region server failures. (Lars H)

        larsh :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK-security #81 (See https://builds.apache.org/job/HBase-TRUNK-security/81/ ) HBASE-5203 Group atomic put/delete operation into a single WALEdit to handle region server failures. (Lars H) larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK #2639 (See https://builds.apache.org/job/HBase-TRUNK/2639/)
        HBASE-5203 Group atomic put/delete operation into a single WALEdit to handle region server failures. (Lars H)

        larsh :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK #2639 (See https://builds.apache.org/job/HBase-TRUNK/2639/ ) HBASE-5203 Group atomic put/delete operation into a single WALEdit to handle region server failures. (Lars H) larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/Delete.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java

          People

          • Assignee:
            lhofhansl Lars Hofhansl
            Reporter:
            lhofhansl Lars Hofhansl
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development