HBase
  1. HBase
  2. HBASE-3584

Allow atomic put/delete in one call

    Details

    • Hadoop Flags:
      Reviewed

      Description

      Right now we have the following calls:

      put(Put)
      delete(Delete)
      increment(Increments)

      But we cannot combine all of the above in a single call, complete with a single row lock. It would be nice to do that.

      It would also allow us to do a CAS where we could do a put/increment if the check succeeded.


      Amendment:
      Since Increment does not currently support MVCC it cannot be included in an atomic operation.
      So this for Put and Delete only.

      1. 3584-v1.txt
        22 kB
        Lars Hofhansl
      2. 3584-v3.txt
        28 kB
        Lars Hofhansl
      3. 3584-final.txt
        29 kB
        Lars Hofhansl

        Issue Links

          Activity

          Hide
          stack added a comment -

          You think this a client API 2.0 feature Ryan?

          Show
          stack added a comment - You think this a client API 2.0 feature Ryan?
          Hide
          ryan rawson added a comment -

          yes API 2.0. Just putting it in here for notability

          Show
          ryan rawson added a comment - yes API 2.0. Just putting it in here for notability
          Hide
          stack added a comment -

          Moving out of 0.92.0. Pull it back in if you think different.

          Show
          stack added a comment - Moving out of 0.92.0. Pull it back in if you think different.
          Hide
          Lars Hofhansl added a comment -

          Should we do this before "client 2.0"?

          What should the client side API look like?
          Something like: AtomicRowOperation (which implements Row, so can itself be batched), which wraps a set Put/Delete/Increment/Append (all for the same rowkey)?

          Compared to a straight put(List<Put>), this would probably be slower as we cannot do the same mini batching (not with a lot of refactoring).

          Also it is not entirely clear that this could even work with Increment/Append of the special immediate-visibility that those need. So maybe start with Put/Delete.

          Show
          Lars Hofhansl added a comment - Should we do this before "client 2.0"? What should the client side API look like? Something like: AtomicRowOperation (which implements Row, so can itself be batched), which wraps a set Put/Delete/Increment/Append (all for the same rowkey)? Compared to a straight put(List<Put>), this would probably be slower as we cannot do the same mini batching (not with a lot of refactoring). Also it is not entirely clear that this could even work with Increment/Append of the special immediate-visibility that those need. So maybe start with Put/Delete.
          Hide
          Lars Hofhansl added a comment -

          Here's an initial patch. Please have a look.
          It includes a simple functional test and a multithreaded "stress test" which also includes region flushes.

          One point I could use some advice on:
          From the stress-test I found that I have to hold the region lock (HRegion.updatesLock.readLock()) for the entire duration of the atomic operation (initially I had thought that would not be necessary).

          The coprocessor pre hooks for delete and put state that they need to be run before the region lock is acquired to avoid deadlocks.

          So... I could (1) disable the pre/post delete and put hooks for this case and invent new hooks for atomic operations, but that would make hard to generally capture puts or deletes.
          Or I could (2) run only the pre-hooks before the atomic mutation, and memorize the results. The post hooks would run as before.

          Show
          Lars Hofhansl added a comment - Here's an initial patch. Please have a look. It includes a simple functional test and a multithreaded "stress test" which also includes region flushes. One point I could use some advice on: From the stress-test I found that I have to hold the region lock (HRegion.updatesLock.readLock()) for the entire duration of the atomic operation (initially I had thought that would not be necessary). The coprocessor pre hooks for delete and put state that they need to be run before the region lock is acquired to avoid deadlocks. So... I could (1) disable the pre/post delete and put hooks for this case and invent new hooks for atomic operations, but that would make hard to generally capture puts or deletes. Or I could (2) run only the pre-hooks before the atomic mutation, and memorize the results. The post hooks would run as before.
          Hide
          Lars Hofhansl added a comment -

          Also this patch only addresses this for Put and Delete. Increment (and Append) do not adhere to mvcc (and cannot be easily made so), so wrapping them into an single atomic operation is (currently) pointless.

          Show
          Lars Hofhansl added a comment - Also this patch only addresses this for Put and Delete. Increment (and Append) do not adhere to mvcc (and cannot be easily made so), so wrapping them into an single atomic operation is (currently) pointless.
          Hide
          Ted Yu added a comment -

          Putting the patch on review board would make reviewing easier.

          Show
          Ted Yu added a comment - Putting the patch on review board would make reviewing easier.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for hbase.

          Summary
          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:
          Delete d = new Delete(ROW);
          Put p = new Put(ROW);
          ...
          AtomicRowMutation arm = new AtomicRowMutation(ROW);
          arm.add(p);
          arm.add(d);
          myHtable.atomicMutation(arm);

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

          Diffs


          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1230675

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

          Testing
          -------

          • Simple functional test: TestFromClientSide.testAtomicRowMutation
          • Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads
          • manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- Review request for hbase. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.atomicMutation(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1230675 Diff: https://reviews.apache.org/r/3481/diff Testing ------- Simple functional test: TestFromClientSide.testAtomicRowMutation Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads manual testing Thanks, Lars
          Hide
          Ted Yu added a comment -

          w.r.t. the coprocessor pre hooks, I think option 2 is better.

          + * The mutations are in performed in the order in which they
          + * were added.
          

          The above should read 'are performed'.

          For atomicMutation(), maybe mutateAtomically is a better name for the method ?

          +          internalDelete((Delete)d, HConstants.DEFAULT_CLUSTER_ID, d.getWriteToWAL(), w);
          +        } else {
          +          throw new DoNotRetryIOException("Action must be Put or Delete");  
          

          Type cast above isn't needed. Please include m in the exception message.

          Show
          Ted Yu added a comment - w.r.t. the coprocessor pre hooks, I think option 2 is better. + * The mutations are in performed in the order in which they + * were added. The above should read 'are performed'. For atomicMutation(), maybe mutateAtomically is a better name for the method ? + internalDelete((Delete)d, HConstants.DEFAULT_CLUSTER_ID, d.getWriteToWAL(), w); + } else { + throw new DoNotRetryIOException( "Action must be Put or Delete" ); Type cast above isn't needed. Please include m in the exception message.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Overall lgtm, but you have a lot of excessive whitespaceing (look for the red in the review)

          • Alex

          On 2012-01-12 21:40:26, Lars Hofhansl wrote:

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

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

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

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

          (Updated 2012-01-12 21:40:26)

          Review request for hbase.

          Summary

          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:

          Delete d = new Delete(ROW);

          Put p = new Put(ROW);

          ...

          AtomicRowMutation arm = new AtomicRowMutation(ROW);

          arm.add(p);

          arm.add(d);

          myHtable.atomicMutation(arm);

          This addresses bug HBASE-3584.

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

          Diffs

          -----

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675

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

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

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

          Testing

          -------

          * Simple functional test: TestFromClientSide.testAtomicRowMutation

          * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads

          * manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/#review4336 ----------------------------------------------------------- Overall lgtm, but you have a lot of excessive whitespaceing (look for the red in the review) Alex On 2012-01-12 21:40:26, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-12 21:40:26) Review request for hbase. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.atomicMutation(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1230675 Diff: https://reviews.apache.org/r/3481/diff Testing ------- * Simple functional test: TestFromClientSide.testAtomicRowMutation * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads * manual testing Thanks, Lars
          Hide
          Lars Hofhansl added a comment -

          Thanks Ted. Looked at the coprocessor API more. Unfortunately both pre and post hooks for Delete and Put take a WALEdit argument; the WALEdit is only created when the Delete/Put happens.
          Could move creation of all WALEdits up in this case and pass WALEdits down, seems nasty.

          It turns out there are some inconsistencies with Put and Delete right now. The Put posthook is executed after the region was released, but the Delete posthook is executed with the region lock held.

          Will address your other comments.

          Show
          Lars Hofhansl added a comment - Thanks Ted. Looked at the coprocessor API more. Unfortunately both pre and post hooks for Delete and Put take a WALEdit argument; the WALEdit is only created when the Delete/Put happens. Could move creation of all WALEdits up in this case and pass WALEdits down, seems nasty. It turns out there are some inconsistencies with Put and Delete right now. The Put posthook is executed after the region was released, but the Delete posthook is executed with the region lock held. Will address your other comments.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2012-01-12 22:10:17.922220)

          Review request for hbase.

          Changes
          -------

          Addressing Alex' and Ted's comment:

          • whitespace
          • renamed atomicMutation to mutateAtomically
          • spelling

          Summary (updated)
          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:
          Delete d = new Delete(ROW);
          Put p = new Put(ROW);
          ...
          AtomicRowMutation arm = new AtomicRowMutation(ROW);
          arm.add(p);
          arm.add(d);
          myHtable.mutateAtomically(arm);

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

          Diffs (updated)


          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1230675

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

          Testing
          -------

          • Simple functional test: TestFromClientSide.testAtomicRowMutation
          • Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads
          • manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-12 22:10:17.922220) Review request for hbase. Changes ------- Addressing Alex' and Ted's comment: whitespace renamed atomicMutation to mutateAtomically spelling Summary (updated) ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.mutateAtomically(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1230675 Diff: https://reviews.apache.org/r/3481/diff Testing ------- Simple functional test: TestFromClientSide.testAtomicRowMutation Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads manual testing Thanks, Lars
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9776>

          Should we consider the case where this.row == null ?
          If possible, please include the two row values in exception message.

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9777>

          Why ignore version ?

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9778>

          Space around semicolon, please.

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          <https://reviews.apache.org/r/3481/#comment9779>

          Can we implement this method ?

          • Ted

          On 2012-01-12 22:10:17, Lars Hofhansl wrote:

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

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

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

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

          (Updated 2012-01-12 22:10:17)

          Review request for hbase.

          Summary

          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:

          Delete d = new Delete(ROW);

          Put p = new Put(ROW);

          ...

          AtomicRowMutation arm = new AtomicRowMutation(ROW);

          arm.add(p);

          arm.add(d);

          myHtable.mutateAtomically(arm);

          This addresses bug HBASE-3584.

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

          Diffs

          -----

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675

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

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

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

          Testing

          -------

          * Simple functional test: TestFromClientSide.testAtomicRowMutation

          * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads

          * manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/#review4341 ----------------------------------------------------------- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9776 > Should we consider the case where this.row == null ? If possible, please include the two row values in exception message. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9777 > Why ignore version ? http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9778 > Space around semicolon, please. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java < https://reviews.apache.org/r/3481/#comment9779 > Can we implement this method ? Ted On 2012-01-12 22:10:17, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-12 22:10:17) Review request for hbase. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.mutateAtomically(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1230675 Diff: https://reviews.apache.org/r/3481/diff Testing ------- * Simple functional test: TestFromClientSide.testAtomicRowMutation * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads * manual testing Thanks, Lars
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2012-01-12 23:59:55.729776)

          Review request for hbase.

          Changes
          -------

          • This version deals correctly with coprocessors.
          • addressed Ted's comments.

          Summary
          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:
          Delete d = new Delete(ROW);
          Put p = new Put(ROW);
          ...
          AtomicRowMutation arm = new AtomicRowMutation(ROW);
          arm.add(p);
          arm.add(d);
          myHtable.mutateAtomically(arm);

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

          Diffs (updated)


          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1230675
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1230675

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

          Testing (updated)
          -------

          • Simple functional test: TestFromClientSide.testAtomicRowMutation
          • Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads
          • coprocessor test: TestRegionObserverInterface.testAtomicRowMutation
          • manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-12 23:59:55.729776) Review request for hbase. Changes ------- This version deals correctly with coprocessors. addressed Ted's comments. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.mutateAtomically(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1230675 Diff: https://reviews.apache.org/r/3481/diff Testing (updated) ------- Simple functional test: TestFromClientSide.testAtomicRowMutation Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads coprocessor test: TestRegionObserverInterface.testAtomicRowMutation manual testing Thanks, Lars
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-01-12 23:23:18, Ted Yu wrote:

          > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java, line 654

          > <https://reviews.apache.org/r/3481/diff/2/?file=68443#file68443line654>

          >

          > Can we implement this method ?

          In a different jira, if there is demand. There are many unsupported methods in here.

          • Lars

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

          On 2012-01-12 23:59:55, Lars Hofhansl wrote:

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

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

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

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

          (Updated 2012-01-12 23:59:55)

          Review request for hbase.

          Summary

          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:

          Delete d = new Delete(ROW);

          Put p = new Put(ROW);

          ...

          AtomicRowMutation arm = new AtomicRowMutation(ROW);

          arm.add(p);

          arm.add(d);

          myHtable.mutateAtomically(arm);

          This addresses bug HBASE-3584.

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

          Diffs

          -----

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1230675

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

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

          Testing

          -------

          * Simple functional test: TestFromClientSide.testAtomicRowMutation

          * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads

          * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation

          * manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-01-12 23:23:18, Ted Yu wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java , line 654 > < https://reviews.apache.org/r/3481/diff/2/?file=68443#file68443line654 > > > Can we implement this method ? In a different jira, if there is demand. There are many unsupported methods in here. Lars ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/#review4341 ----------------------------------------------------------- On 2012-01-12 23:59:55, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-12 23:59:55) Review request for hbase. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.mutateAtomically(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1230675 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1230675 Diff: https://reviews.apache.org/r/3481/diff Testing ------- * Simple functional test: TestFromClientSide.testAtomicRowMutation * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation * manual testing Thanks, Lars
          Hide
          Amitanand Aiyer added a comment -

          Nice feature, lars!!

          Would be nice to have a batch/list functionality as well, like we have for deletes/puts.

          Are there any (future?) plans to have a multi- version of the atomicMutation?

          Show
          Amitanand Aiyer added a comment - Nice feature, lars!! Would be nice to have a batch/list functionality as well, like we have for deletes/puts. Are there any (future?) plans to have a multi- version of the atomicMutation?
          Hide
          Lars Hofhansl added a comment -

          Hi Amit... Since AtomicRowMutation implements the Row interface batching would be done with HTable.batch (which takes a List<Row> and translates to an HRegionServer.multi request). Is that what you meant?

          Not entirely sure what you mean with multi-version atomicMutation. Multiple versions is already possible by adding multiple Puts/Deletes with different versions).
          Or do you mean atomic operations across multiple different rows? That is not going to happen any time soon

          Show
          Lars Hofhansl added a comment - Hi Amit... Since AtomicRowMutation implements the Row interface batching would be done with HTable.batch (which takes a List<Row> and translates to an HRegionServer.multi request). Is that what you meant? Not entirely sure what you mean with multi-version atomicMutation. Multiple versions is already possible by adding multiple Puts/Deletes with different versions). Or do you mean atomic operations across multiple different rows? That is not going to happen any time soon
          Hide
          Lars Hofhansl added a comment -

          This is the same patch that is up on RB right now. Getting a test run in.

          Show
          Lars Hofhansl added a comment - This is the same patch that is up on RB right now. Getting a test run in.
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 javadoc. The javadoc tool appears to have generated -147 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.coprocessor.TestRegionObserverInterface
          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/752//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/752//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/752//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/12510460/3584-v3.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -147 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.coprocessor.TestRegionObserverInterface 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/752//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/752//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/752//console This message is automatically generated.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2012-01-13 17:11:21.333184)

          Review request for hbase.

          Changes
          -------

          This version passes all tests.
          Fixed some Java doc comments.

          This is a good candidate for commit.

          Summary
          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:
          Delete d = new Delete(ROW);
          Put p = new Put(ROW);
          ...
          AtomicRowMutation arm = new AtomicRowMutation(ROW);
          arm.add(p);
          arm.add(d);
          myHtable.mutateAtomically(arm);

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

          Diffs (updated)


          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1231172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1231172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1231172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231172
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231172

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

          Testing
          -------

          • Simple functional test: TestFromClientSide.testAtomicRowMutation
          • Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads
          • coprocessor test: TestRegionObserverInterface.testAtomicRowMutation
          • manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-13 17:11:21.333184) Review request for hbase. Changes ------- This version passes all tests. Fixed some Java doc comments. This is a good candidate for commit. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.mutateAtomically(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231172 Diff: https://reviews.apache.org/r/3481/diff Testing ------- Simple functional test: TestFromClientSide.testAtomicRowMutation Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads coprocessor test: TestRegionObserverInterface.testAtomicRowMutation manual testing Thanks, Lars
          Hide
          Ted Yu added a comment -

          The coprocessor hooks are contingent upon this condition:

          walEdit == null
          

          I think this is different from current practice.

          Can we instantiate WALEdit locally and use it when coprocessorHost != null ?

          Show
          Ted Yu added a comment - The coprocessor hooks are contingent upon this condition: walEdit == null I think this is different from current practice. Can we instantiate WALEdit locally and use it when coprocessorHost != null ?
          Hide
          Lars Hofhansl added a comment -

          Hmm... Somehow I have to indicate that coprocessor hooks are executed by the caller.
          In the special case of mutateAtomically the caller has to handle the coprocessor hooks (to avoid deadlocks).

          If coprocessorHost != null and this is a call from mutateAtomically we cannot instantiate the WALEdit locally, because it is needed by the coprocessor hooks (who could change it).

          walEdit is the one passed in, it is null unless the call is from mutateAtomically.
          The one actually used is localWalEdit. Maybe I should rename walEdit to passedWalEdit?

          Show
          Lars Hofhansl added a comment - Hmm... Somehow I have to indicate that coprocessor hooks are executed by the caller. In the special case of mutateAtomically the caller has to handle the coprocessor hooks (to avoid deadlocks). If coprocessorHost != null and this is a call from mutateAtomically we cannot instantiate the WALEdit locally, because it is needed by the coprocessor hooks (who could change it). walEdit is the one passed in, it is null unless the call is from mutateAtomically. The one actually used is localWalEdit. Maybe I should rename walEdit to passedWalEdit?
          Hide
          Ted Yu added a comment -

          How about we always instantiate WALEdit, called localWalEdit.
          We have the passedWalEdit.
          And we maintain effectiveWalEdit which would be:

          passedWalEdit == null ? localWalEdit : passedWalEdit
          

          We use localWalEdit for coprocessor hook and effectiveWalEdit for addFamilyMapToWALEdit() and log.append()

          Show
          Ted Yu added a comment - How about we always instantiate WALEdit, called localWalEdit. We have the passedWalEdit. And we maintain effectiveWalEdit which would be: passedWalEdit == null ? localWalEdit : passedWalEdit We use localWalEdit for coprocessor hook and effectiveWalEdit for addFamilyMapToWALEdit() and log.append()
          Hide
          Lars Hofhansl added a comment -

          It's kind of how it works now. If walEdit is passed in non-null, use that one (and also assume that the caller used that same walEdit to execute the pre-hooks and will use it execute the post hooks), otherwise make a local one.

          If there is a non-null passedWalEdit, that same one has to be used for addFamilyMapToWALEdit() and log.append().

          In the end it is similar to passing the writeEntry in. The caller perform the grouping of the operations, so it has to control what writeEntry and what walEdits to use.

          Would it be less confusing to pass in an extra flag to indicate whether the coprocessor hooks need to be executed?

          Show
          Lars Hofhansl added a comment - It's kind of how it works now. If walEdit is passed in non-null, use that one (and also assume that the caller used that same walEdit to execute the pre-hooks and will use it execute the post hooks), otherwise make a local one. If there is a non-null passedWalEdit, that same one has to be used for addFamilyMapToWALEdit() and log.append(). In the end it is similar to passing the writeEntry in. The caller perform the grouping of the operations, so it has to control what writeEntry and what walEdits to use. Would it be less confusing to pass in an extra flag to indicate whether the coprocessor hooks need to be executed?
          Hide
          Lars Hofhansl added a comment -

          Oh... and this is the piece of code that does this:

          WALEdit localWalEdit = walEdit == null ? new WALEdit() : walEdit;
          

          Inside internal

          {Put|Delete}

          it is always using localWalEdit, except for deciding whether the coprocessor hooks need to be executed (where it used walEdit==null as a flag).

          Show
          Lars Hofhansl added a comment - Oh... and this is the piece of code that does this: WALEdit localWalEdit = walEdit == null ? new WALEdit() : walEdit; Inside internal {Put|Delete} it is always using localWalEdit, except for deciding whether the coprocessor hooks need to be executed (where it used walEdit==null as a flag).
          Hide
          Ted Yu added a comment -

          The current logic is different from what I described @ 13/Jan/12 19:06.
          With 3 xxWalEdit's, coprocessor hooks would always be executed, consistent with current practice.

          I think the above is cleaner compared to introducing a boolean.

          Show
          Ted Yu added a comment - The current logic is different from what I described @ 13/Jan/12 19:06. With 3 xxWalEdit's, coprocessor hooks would always be executed, consistent with current practice. I think the above is cleaner compared to introducing a boolean.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          +1 with some comments below.

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9816>

          You think this class needs to be AtomicRowMutation? (makes me want to 'duck and cover'). All mutations on a row in hbase are 'atomic' so the prefix strikes me as redundant

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9817>

          Yeah, its kinda crazy all this duplication of say the row byte array.. it'll be here and then up in Put and Delete. Its like we need a shorthand of some kind.... but for another time.

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9818>

          Should be 0? (No biggie)

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9819>

          Why have an add for Put and one for Delete and not just an add Mutation?

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9820>

          Important!

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9821>

          Usually we add space around operators. Next time.

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9822>

          Good

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          <https://reviews.apache.org/r/3481/#comment9823>

          Call it mutate?

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

          mutate?

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

          Duplicated code? We should fix this up some day

          • Michael

          On 2012-01-13 17:11:21, Lars Hofhansl wrote:

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

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

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

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

          (Updated 2012-01-13 17:11:21)

          Review request for hbase.

          Summary

          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:

          Delete d = new Delete(ROW);

          Put p = new Put(ROW);

          ...

          AtomicRowMutation arm = new AtomicRowMutation(ROW);

          arm.add(p);

          arm.add(d);

          myHtable.mutateAtomically(arm);

          This addresses bug HBASE-3584.

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

          Diffs

          -----

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172

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

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

          Testing

          -------

          * Simple functional test: TestFromClientSide.testAtomicRowMutation

          * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads

          * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation

          * manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/#review4368 ----------------------------------------------------------- +1 with some comments below. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9816 > You think this class needs to be AtomicRowMutation? (makes me want to 'duck and cover'). All mutations on a row in hbase are 'atomic' so the prefix strikes me as redundant http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9817 > Yeah, its kinda crazy all this duplication of say the row byte array.. it'll be here and then up in Put and Delete. Its like we need a shorthand of some kind.... but for another time. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9818 > Should be 0? (No biggie) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9819 > Why have an add for Put and one for Delete and not just an add Mutation? http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9820 > Important! http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9821 > Usually we add space around operators. Next time. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9822 > Good http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java < https://reviews.apache.org/r/3481/#comment9823 > Call it mutate? http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/3481/#comment9824 > mutate? http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/3481/#comment9825 > Duplicated code? We should fix this up some day Michael On 2012-01-13 17:11:21, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-13 17:11:21) Review request for hbase. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.mutateAtomically(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231172 Diff: https://reviews.apache.org/r/3481/diff Testing ------- * Simple functional test: TestFromClientSide.testAtomicRowMutation * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation * manual testing Thanks, Lars
          Hide
          Lars Hofhansl added a comment -

          Now I am getting confused
          These are the parameters:
          1. mutateAtomically needs to run the pre/post coprocessor hooks (so internal

          {Put|Delete} should not run the pre/post hooks when called from mutateAtomically).
          2. the walEdits used on the pre/post hooks must the same as the ones used in addFamilyMapToWALEdit()/log.append()
          3. if not called from mutateAtomically internal{Put|Delete}

          are responsible for executing the pre/post hooks (as currently done), using a local walEdit.

          Show
          Lars Hofhansl added a comment - Now I am getting confused These are the parameters: 1. mutateAtomically needs to run the pre/post coprocessor hooks (so internal {Put|Delete} should not run the pre/post hooks when called from mutateAtomically). 2. the walEdits used on the pre/post hooks must the same as the ones used in addFamilyMapToWALEdit()/log.append() 3. if not called from mutateAtomically internal{Put|Delete} are responsible for executing the pre/post hooks (as currently done), using a local walEdit.
          Hide
          Ted Yu added a comment -

          I agree.
          Sorry for the confusion.

          Show
          Ted Yu added a comment - I agree. Sorry for the confusion.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9827>

          Yep... The same that way that all KVs in Put/Delete's familyMap replicate the same rowkey... Should tackle that generally in a separate jira.

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9828>

          Versions are 0-based?

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9829>

          Append is also a Mutation and is not supported (neither is Increment, but that's not a Mutation).
          If/when we support all Mutation we can have a general add(Mutation) mutation (nice thing that this won't change the visible client api)

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9830>

          Yep!

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java
          <https://reviews.apache.org/r/3481/#comment9831>

          Will change

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          <https://reviews.apache.org/r/3481/#comment9832>

          Hmm... Torn about this.
          The main point of this is that it is atomic. We already have multiAction (which can take Mutations), but it is not atomic.

          • Lars

          On 2012-01-13 17:11:21, Lars Hofhansl wrote:

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

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

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

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

          (Updated 2012-01-13 17:11:21)

          Review request for hbase.

          Summary

          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:

          Delete d = new Delete(ROW);

          Put p = new Put(ROW);

          ...

          AtomicRowMutation arm = new AtomicRowMutation(ROW);

          arm.add(p);

          arm.add(d);

          myHtable.mutateAtomically(arm);

          This addresses bug HBASE-3584.

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

          Diffs

          -----

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172

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

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

          Testing

          -------

          * Simple functional test: TestFromClientSide.testAtomicRowMutation

          * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads

          * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation

          * manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/#review4370 ----------------------------------------------------------- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9827 > Yep... The same that way that all KVs in Put/Delete's familyMap replicate the same rowkey... Should tackle that generally in a separate jira. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9828 > Versions are 0-based? http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9829 > Append is also a Mutation and is not supported (neither is Increment, but that's not a Mutation). If/when we support all Mutation we can have a general add(Mutation) mutation (nice thing that this won't change the visible client api) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9830 > Yep! http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java < https://reviews.apache.org/r/3481/#comment9831 > Will change http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java < https://reviews.apache.org/r/3481/#comment9832 > Hmm... Torn about this. The main point of this is that it is atomic. We already have multiAction (which can take Mutations), but it is not atomic. Lars On 2012-01-13 17:11:21, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-13 17:11:21) Review request for hbase. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.mutateAtomically(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231172 Diff: https://reviews.apache.org/r/3481/diff Testing ------- * Simple functional test: TestFromClientSide.testAtomicRowMutation * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation * manual testing Thanks, Lars
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-01-13 20:17:55, Lars Hofhansl wrote:

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

          > <https://reviews.apache.org/r/3481/diff/4/?file=68809#file68809line755>

          >

          > Hmm... Torn about this.

          > The main point of this is that it is atomic. We already have multiAction (which can take Mutations), but it is not atomic.

          >

          Ok

          MultiAction is x-row, right? RowMutation has the row prefix.

          But if you think it makes it clearer, go for it.

          • Michael

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

          On 2012-01-13 17:11:21, Lars Hofhansl wrote:

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

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

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

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

          (Updated 2012-01-13 17:11:21)

          Review request for hbase.

          Summary

          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:

          Delete d = new Delete(ROW);

          Put p = new Put(ROW);

          ...

          AtomicRowMutation arm = new AtomicRowMutation(ROW);

          arm.add(p);

          arm.add(d);

          myHtable.mutateAtomically(arm);

          This addresses bug HBASE-3584.

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

          Diffs

          -----

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172

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

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

          Testing

          -------

          * Simple functional test: TestFromClientSide.testAtomicRowMutation

          * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads

          * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation

          * manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-01-13 20:17:55, Lars Hofhansl wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java , line 755 > < https://reviews.apache.org/r/3481/diff/4/?file=68809#file68809line755 > > > Hmm... Torn about this. > The main point of this is that it is atomic. We already have multiAction (which can take Mutations), but it is not atomic. > Ok MultiAction is x-row, right? RowMutation has the row prefix. But if you think it makes it clearer, go for it. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/#review4370 ----------------------------------------------------------- On 2012-01-13 17:11:21, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-13 17:11:21) Review request for hbase. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.mutateAtomically(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231172 Diff: https://reviews.apache.org/r/3481/diff Testing ------- * Simple functional test: TestFromClientSide.testAtomicRowMutation * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation * manual testing Thanks, Lars
          Hide
          Lars Hofhansl added a comment -

          Wanna call it mutateRow (in HTable/RegionServer/Region)? That would match the RowMutation object name.

          Is this good enough for commit, or are you guys unsure?

          Show
          Lars Hofhansl added a comment - Wanna call it mutateRow (in HTable/RegionServer/Region)? That would match the RowMutation object name. Is this good enough for commit, or are you guys unsure?
          Hide
          stack added a comment -

          Wanna call it mutateRow (in HTable/RegionServer/Region)? That would match the RowMutation object name.

          Fine by me. You going to change the class name too?

          I'm fine w/ commit.

          Show
          stack added a comment - Wanna call it mutateRow (in HTable/RegionServer/Region)? That would match the RowMutation object name. Fine by me. You going to change the class name too? I'm fine w/ commit.
          Hide
          Lars Hofhansl added a comment -

          Yep... HTable.mutateRow(RowMutation)

          Show
          Lars Hofhansl added a comment - Yep... HTable.mutateRow(RowMutation)
          Hide
          Lars Hofhansl added a comment -

          Patch I would like to commit.
          Renamed methods and classes (along with test names, etc) as discussed with Stack.

          Ted, you OK with this patch?

          Show
          Lars Hofhansl added a comment - Patch I would like to commit. Renamed methods and classes (along with test names, etc) as discussed with Stack. Ted, you OK with this patch?
          Hide
          Ted Yu added a comment -

          +1 if unit tests pass.

          Nice work, Lars.

          Show
          Ted Yu added a comment - +1 if unit tests pass. Nice work, Lars.
          Hide
          Amitanand Aiyer added a comment -

          Thanks Lars. I was indeed referring to the batching that you were talking about.

          Never mind about the multi- version of the atomicMutation. Was stuck thinking about 89 in my mind. Was wondering if we might need another function that takes a list of AtomicRowMutations, as an input. As you point out, HTable.batch should do this in trunk.

          Show
          Amitanand Aiyer added a comment - Thanks Lars. I was indeed referring to the batching that you were talking about. Never mind about the multi- version of the atomicMutation. Was stuck thinking about 89 in my mind. Was wondering if we might need another function that takes a list of AtomicRowMutations, as an input. As you point out, HTable.batch should do this in trunk.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 9 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.coprocessor.TestMasterObserver
          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/757//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/757//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/757//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/12510551/3584-final.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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.coprocessor.TestMasterObserver 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/757//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/757//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/757//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Ran the 4 failing tests locally. All pass. Will commit later this evening.

          @Amit: Cool. Let me know if you think there's some other functionality missing.

          Show
          Lars Hofhansl added a comment - Ran the 4 failing tests locally. All pass. Will commit later this evening. @Amit: Cool. Let me know if you think there's some other functionality missing.
          Hide
          Lars Hofhansl added a comment -

          Changed title, amended description.

          Show
          Lars Hofhansl added a comment - Changed title, amended description.
          Hide
          Lars Hofhansl added a comment -

          Committed to trunk. Thanks for the reviews as usual!

          Show
          Lars Hofhansl added a comment - Committed to trunk. Thanks for the reviews as usual!
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #78 (See https://builds.apache.org/job/HBase-TRUNK-security/78/)
          HBASE-3584 Allow atomic put/delete in one call (Lars H)

          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #78 (See https://builds.apache.org/job/HBase-TRUNK-security/78/ ) HBASE-3584 Allow atomic put/delete in one call (Lars H) larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2633 (See https://builds.apache.org/job/HBase-TRUNK/2633/)
          HBASE-3584 Allow atomic put/delete in one call (Lars H)

          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2633 (See https://builds.apache.org/job/HBase-TRUNK/2633/ ) HBASE-3584 Allow atomic put/delete in one call (Lars H) larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Hide
          Lars Hofhansl added a comment - - edited

          I noticed that there is a hole in my logic when faced with region server failures.

          Since the WALEdits are applied to the WAL piece by piece what can happen is that the region server applied the WALEdits of some of the mutations, and then dies before it can apply the edits of the rest of the mutations.

          The region server who is picking up the region will replay those edit and hence clients picking up the KVs from the recovering region server will see the operation partially applied.
          The fact that the MVCC write point was not forwarded by the first region server is irrelevant and not known to the recovering region server.

          Other operation (Increment/Append) also have correctness issues (albeit different) in failure scenarios.

          Filed HBASE-5203. Work on this would not be trivial. What's the general feeling: Disable this feature until this is fixed? Or leave it enabled, since it is in principle not safer than Increment or ICV?

          Show
          Lars Hofhansl added a comment - - edited I noticed that there is a hole in my logic when faced with region server failures. Since the WALEdits are applied to the WAL piece by piece what can happen is that the region server applied the WALEdits of some of the mutations, and then dies before it can apply the edits of the rest of the mutations. The region server who is picking up the region will replay those edit and hence clients picking up the KVs from the recovering region server will see the operation partially applied. The fact that the MVCC write point was not forwarded by the first region server is irrelevant and not known to the recovering region server. Other operation (Increment/Append) also have correctness issues (albeit different) in failure scenarios. Filed HBASE-5203 . Work on this would not be trivial. What's the general feeling: Disable this feature until this is fixed? Or leave it enabled, since it is in principle not safer than Increment or ICV?
          Hide
          stack added a comment -

          Leave it enabled but document the hole, preferably in javadoc? (Make a new issue to do this?)

          Show
          stack added a comment - Leave it enabled but document the hole, preferably in javadoc? (Make a new issue to do this?)
          Hide
          Lars Hofhansl added a comment -

          How about I'll do an addendum patch updating the Javadoc under this jira?

          Then with HBASE-5203 I'll remove that Javadoc when it's done.

          Show
          Lars Hofhansl added a comment - How about I'll do an addendum patch updating the Javadoc under this jira? Then with HBASE-5203 I'll remove that Javadoc when it's done.
          Hide
          Ted Yu added a comment -

          I think we should temporarily hide the new API.
          mutateRow() is currently only in TRUNK. By the time 0.94 is released, HBASE-5203 should have been tackled.
          It is better to give users a fully functional API.

          Show
          Ted Yu added a comment - I think we should temporarily hide the new API. mutateRow() is currently only in TRUNK. By the time 0.94 is released, HBASE-5203 should have been tackled. It is better to give users a fully functional API.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

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

          @Lars: Sorry about the late question. I would like to understand the following case.

          What if we crash in between this loop?

          Is it possible that we have persisted half the WALEdits, but not gotten to the rest?

          I am concerned, that if that were the case (hopefully not) then on RS restart, we will replay, a partial list of mutations from "arm".

          • Amitanand

          On 2012-01-13 17:11:21, Lars Hofhansl wrote:

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

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

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

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

          (Updated 2012-01-13 17:11:21)

          Review request for hbase.

          Summary

          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:

          Delete d = new Delete(ROW);

          Put p = new Put(ROW);

          ...

          AtomicRowMutation arm = new AtomicRowMutation(ROW);

          arm.add(p);

          arm.add(d);

          myHtable.mutateAtomically(arm);

          This addresses bug HBASE-3584.

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

          Diffs

          -----

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172

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

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

          Testing

          -------

          * Simple functional test: TestFromClientSide.testAtomicRowMutation

          * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads

          * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation

          * manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/#review4445 ----------------------------------------------------------- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/3481/#comment9976 > @Lars: Sorry about the late question. I would like to understand the following case. What if we crash in between this loop? Is it possible that we have persisted half the WALEdits, but not gotten to the rest? I am concerned, that if that were the case (hopefully not) then on RS restart, we will replay, a partial list of mutations from "arm". Amitanand On 2012-01-13 17:11:21, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-13 17:11:21) Review request for hbase. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.mutateAtomically(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231172 Diff: https://reviews.apache.org/r/3481/diff Testing ------- * Simple functional test: TestFromClientSide.testAtomicRowMutation * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation * manual testing Thanks, Lars
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-01-18 21:18:16, Amitanand Aiyer wrote:

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

          > <https://reviews.apache.org/r/3481/diff/4/?file=68814#file68814line4187>

          >

          > @Lars: Sorry about the late question. I would like to understand the following case.

          >

          > What if we crash in between this loop?

          >

          > Is it possible that we have persisted half the WALEdits, but not gotten to the rest?

          >

          > I am concerned, that if that were the case (hopefully not) then on RS restart, we will replay, a partial list of mutations from "arm".

          >

          >

          >

          >

          Yes, which is why I filed, implemented, and committed HBASE-5203, which fixes the problem for real

          • Lars

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

          On 2012-01-13 17:11:21, Lars Hofhansl wrote:

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

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

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

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

          (Updated 2012-01-13 17:11:21)

          Review request for hbase.

          Summary

          -------

          Add API for atomic row mutations to HBase (currently Put and Delete).

          Client API would look like this:

          Delete d = new Delete(ROW);

          Put p = new Put(ROW);

          ...

          AtomicRowMutation arm = new AtomicRowMutation(ROW);

          arm.add(p);

          arm.add(d);

          myHtable.mutateAtomically(arm);

          This addresses bug HBASE-3584.

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

          Diffs

          -----

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172

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

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172

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

          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172

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

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

          Testing

          -------

          * Simple functional test: TestFromClientSide.testAtomicRowMutation

          * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads

          * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation

          * manual testing

          Thanks,

          Lars

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-01-18 21:18:16, Amitanand Aiyer wrote: > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java , line 4187 > < https://reviews.apache.org/r/3481/diff/4/?file=68814#file68814line4187 > > > @Lars: Sorry about the late question. I would like to understand the following case. > > What if we crash in between this loop? > > Is it possible that we have persisted half the WALEdits, but not gotten to the rest? > > I am concerned, that if that were the case (hopefully not) then on RS restart, we will replay, a partial list of mutations from "arm". > > > > Yes, which is why I filed, implemented, and committed HBASE-5203 , which fixes the problem for real Lars ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/#review4445 ----------------------------------------------------------- On 2012-01-13 17:11:21, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3481/ ----------------------------------------------------------- (Updated 2012-01-13 17:11:21) Review request for hbase. Summary ------- Add API for atomic row mutations to HBase (currently Put and Delete). Client API would look like this: Delete d = new Delete(ROW); Put p = new Put(ROW); ... AtomicRowMutation arm = new AtomicRowMutation(ROW); arm.add(p); arm.add(d); myHtable.mutateAtomically(arm); This addresses bug HBASE-3584 . https://issues.apache.org/jira/browse/HBASE-3584 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AtomicRowMutation.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 1231172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1231172 Diff: https://reviews.apache.org/r/3481/diff Testing ------- * Simple functional test: TestFromClientSide.testAtomicRowMutation * Multithreaded stress test: TestAtomicOperation.testAtomicMutationMultiThreads * coprocessor test: TestRegionObserverInterface.testAtomicRowMutation * manual testing Thanks, Lars
          Hide
          Kannan Muthukkaruppan added a comment -

          Lars, Stack:

          As part of HBASE-3967 (https://issues.apache.org/jira/secure/attachment/12482718/diff.patch) we already added a RowMutation in HBase client for bulk import support for deletes. That is yet to be ported to trunk. Would you folks be ok if we rename RowMutation in trunk to something else (say RowChange)? We already have application level code relying on the RowMutation for the bulk import pipelines. So wondering if that would be ok?

          Amitanand: FYI.

          Show
          Kannan Muthukkaruppan added a comment - Lars, Stack: As part of HBASE-3967 ( https://issues.apache.org/jira/secure/attachment/12482718/diff.patch ) we already added a RowMutation in HBase client for bulk import support for deletes. That is yet to be ported to trunk. Would you folks be ok if we rename RowMutation in trunk to something else (say RowChange)? We already have application level code relying on the RowMutation for the bulk import pipelines. So wondering if that would be ok? Amitanand: FYI.
          Hide
          Lars Hofhansl added a comment -

          Is there a way to unify the two? They full fill the same purpose: group puts and deletes together.
          Not at a computer right now, but I can take a look later.
          Otherwise I have no problem renaming. Maybe into something that indicates a grouping of operations for a row.

          Show
          Lars Hofhansl added a comment - Is there a way to unify the two? They full fill the same purpose: group puts and deletes together. Not at a computer right now, but I can take a look later. Otherwise I have no problem renaming. Maybe into something that indicates a grouping of operations for a row.
          Hide
          stack added a comment -

          Looks like HBASE-3967 RowMutation is a common denominator of Put and Delete where as this RowMutation is a container that holds a swathe of Puts and Deletes to apply to a single row; they are different.

          I'm up for rename. This RowMutation becomes RowMutations?

          Show
          stack added a comment - Looks like HBASE-3967 RowMutation is a common denominator of Put and Delete where as this RowMutation is a container that holds a swathe of Puts and Deletes to apply to a single row; they are different. I'm up for rename. This RowMutation becomes RowMutations?
          Hide
          Lars Hofhansl added a comment -

          At a computer now. Stack you're right RowMutation of HBASE-3967 encapsulates either a Put or a Delete.
          In trunk we have Mutation now as common superclass to Put and Delete, that class should be used to indicate either Put or Delete (the way I did it in HBASE-4682):

             static class Importer
          -  extends TableMapper<ImmutableBytesWritable, Put> {
          +  extends TableMapper<ImmutableBytesWritable, Mutation> {
               private Map<byte[], byte[]> cfRenameMap;
          

          I think this gets very confusing. We have Mutation as superclass of Put and Delete, RowMutation to be either a Delete or a Put, and RowMutations, which is a grouping of operations for the same Row.
          Kannan, can you use Mutation.java from trunk for the MR jobs they way I did in HBASE-4682?

          Show
          Lars Hofhansl added a comment - At a computer now. Stack you're right RowMutation of HBASE-3967 encapsulates either a Put or a Delete. In trunk we have Mutation now as common superclass to Put and Delete, that class should be used to indicate either Put or Delete (the way I did it in HBASE-4682 ): static class Importer - extends TableMapper<ImmutableBytesWritable, Put> { + extends TableMapper<ImmutableBytesWritable, Mutation> { private Map< byte [], byte []> cfRenameMap; I think this gets very confusing. We have Mutation as superclass of Put and Delete, RowMutation to be either a Delete or a Put, and RowMutations, which is a grouping of operations for the same Row. Kannan, can you use Mutation.java from trunk for the MR jobs they way I did in HBASE-4682 ?
          Hide
          Lars Hofhansl added a comment -

          @Kannan, I guess you said already that that would be tricky for you.

          Show
          Lars Hofhansl added a comment - @Kannan, I guess you said already that that would be tricky for you.
          Hide
          Ted Yu added a comment -

          I looked at 89-fb/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java
          It is not versioned. And write(DataOutput out) has:

              out.writeLong(RowMutation.globalOrderCounter.incrementAndGet());
          

          The divergence is noticeable.
          If RowMutation in 89-fb were versioned, we may be able to assign a higher version number to RowMutation in TRUNK.
          But that's not pretty either.

          Show
          Ted Yu added a comment - I looked at 89-fb/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java It is not versioned. And write(DataOutput out) has: out.writeLong(RowMutation.globalOrderCounter.incrementAndGet()); The divergence is noticeable. If RowMutation in 89-fb were versioned, we may be able to assign a higher version number to RowMutation in TRUNK. But that's not pretty either.
          Hide
          Lars Hofhansl added a comment -

          @Ted: I think the two are too different to make them two versions of the same thing.
          Ideally Kannan would use the Mutation object that I introduced in 0.92. If that is too risky (due to required production code changes) we should change the trunk's RowMutation to something else.

          Possible new names:

          • RowMutations (as Stack suggests)
          • RowChanges (as Kannan suggests)
          • GroupedOperations
          • AtomicOperation
          • AtomicMutations
          • Transaction (might wanna reserve this for later, though)
          • MutationGroup

          Or be radical and remove the trunk version of RowMutation alltogether and just use List<Mutation> (which is in essence all that trunk's RowMutation is anyway, downside is that then we couldn't limit it to Put and Delete - Increment is also a Mutation).

          Show
          Lars Hofhansl added a comment - @Ted: I think the two are too different to make them two versions of the same thing. Ideally Kannan would use the Mutation object that I introduced in 0.92. If that is too risky (due to required production code changes) we should change the trunk's RowMutation to something else. Possible new names: RowMutations (as Stack suggests) RowChanges (as Kannan suggests) GroupedOperations AtomicOperation AtomicMutations Transaction (might wanna reserve this for later, though) MutationGroup Or be radical and remove the trunk version of RowMutation alltogether and just use List<Mutation> (which is in essence all that trunk's RowMutation is anyway, downside is that then we couldn't limit it to Put and Delete - Increment is also a Mutation).
          Hide
          Ted Yu added a comment -

          Since RowMutation in both code bases only represent Put and Delete, introducing List<Mutation> in this context is indeed radical.

          Show
          Ted Yu added a comment - Since RowMutation in both code bases only represent Put and Delete, introducing List<Mutation> in this context is indeed radical.
          Hide
          Lars Hofhansl added a comment -

          @Ted: HBASE-3967's RowMutation describes a single operation which is either a Put or a Delete. Trunk's RowMutation describes a set of operation each of which can be either a Put or a Delete.

          Show
          Lars Hofhansl added a comment - @Ted: HBASE-3967 's RowMutation describes a single operation which is either a Put or a Delete. Trunk's RowMutation describes a set of operation each of which can be either a Put or a Delete.
          Hide
          Kannan Muthukkaruppan added a comment -

          Lars: Yes... it would be tricky since we have some app level code already relying on RowMutation.

          Regarding name suggestions, RowMutations sounds good.. since one is likely to use this only when you typically have a more than one mutation (typically a mix of deletes and puts). RowChanges or RowOperations would be some alternatives. How about RowOperations (if we feel RowMutations vs RowMutation is too similar/confusing).

          Show
          Kannan Muthukkaruppan added a comment - Lars: Yes... it would be tricky since we have some app level code already relying on RowMutation. Regarding name suggestions, RowMutations sounds good.. since one is likely to use this only when you typically have a more than one mutation (typically a mix of deletes and puts). RowChanges or RowOperations would be some alternatives. How about RowOperations (if we feel RowMutations vs RowMutation is too similar/confusing).
          Hide
          Ted Yu added a comment -

          RowMutation isn't in 0.92 so there shouldn't be much issue with renaming.

          I suggest going over new public classes in TRUNK to see if there is any more that is in conflict with same class from 0.89-fb (maybe Kannan has done this)

          Show
          Ted Yu added a comment - RowMutation isn't in 0.92 so there shouldn't be much issue with renaming. I suggest going over new public classes in TRUNK to see if there is any more that is in conflict with same class from 0.89-fb (maybe Kannan has done this)
          Hide
          stack added a comment -

          This is an unfortunate situation – e.g. Mutation is in 0.92 release and its what RowMutation is in 0.89fb IIUC – but lets patch up the model as best we can with deprecations of the ugly so we're clear on what we want folks to use going forward (I believe I reviewed both patches – I should have caught the name clash).

          Show
          stack added a comment - This is an unfortunate situation – e.g. Mutation is in 0.92 release and its what RowMutation is in 0.89fb IIUC – but lets patch up the model as best we can with deprecations of the ugly so we're clear on what we want folks to use going forward (I believe I reviewed both patches – I should have caught the name clash).
          Hide
          Lars Hofhansl added a comment -

          Let's just rename trunk's RowMutation to RowMutations.
          @Kannan: We should probably have a separate jira for that.

          Show
          Lars Hofhansl added a comment - Let's just rename trunk's RowMutation to RowMutations. @Kannan: We should probably have a separate jira for that.
          Hide
          stack added a comment -

          Let's just rename trunk's RowMutation to RowMutations.

          Sure, but don't we want to also deprecate RowMutation when it goes in and point people to instead use Mutation?

          Show
          stack added a comment - Let's just rename trunk's RowMutation to RowMutations. Sure, but don't we want to also deprecate RowMutation when it goes in and point people to instead use Mutation?
          Hide
          Ted Yu added a comment -

          I think some users would ask what the relationship between RowMutations and RowMutation is.

          Show
          Ted Yu added a comment - I think some users would ask what the relationship between RowMutations and RowMutation is.
          Hide
          Lars Hofhansl added a comment -

          @Stack and Ted: Yes to both.
          The core problem is that FB has production code out there that uses RowMutation and that at the same time that code contribution should be in trunk.
          So it seems that either:

          1. We accept that patch into trunk and rename RowMutation, and deprecate the new RowMutation, etc.
          2. Do not accept the code. Then FB can either change their code or not contribute that patch.

          I'd rather have #1.
          @Kannan: Do you think you can ever change that code on your side? If, yes, we can either wait for that to happen, or depreate your RowMutation from the start, and then remove it when you made the production change.

          Show
          Lars Hofhansl added a comment - @Stack and Ted: Yes to both. The core problem is that FB has production code out there that uses RowMutation and that at the same time that code contribution should be in trunk. So it seems that either: We accept that patch into trunk and rename RowMutation, and deprecate the new RowMutation, etc. Do not accept the code. Then FB can either change their code or not contribute that patch. I'd rather have #1. @Kannan: Do you think you can ever change that code on your side? If, yes, we can either wait for that to happen, or depreate your RowMutation from the start, and then remove it when you made the production change.
          Hide
          stack added a comment -

          I'm +1 on #1

          Show
          stack added a comment - I'm +1 on #1
          Hide
          Kannan Muthukkaruppan added a comment -

          Lars/Stack: So you are ok, if we submit a patch to rename RowMutation in trunk to RowMutations? [I didn't understand the part about deprecating RowMutation-- that's currently only in 89-fb branch correct? We'll try to forward port that (HBASE-3967) to trunk as well if that's ok.

          Show
          Kannan Muthukkaruppan added a comment - Lars/Stack: So you are ok, if we submit a patch to rename RowMutation in trunk to RowMutations? [I didn't understand the part about deprecating RowMutation-- that's currently only in 89-fb branch correct? We'll try to forward port that ( HBASE-3967 ) to trunk as well if that's ok.
          Hide
          stack added a comment -

          @Kannan Yes. Sounds good. Was saying that the forwarded ported RowMutation should be deprecated on forward-port since it does what Mutation is in TRUNK – but we can that in another, subsequent patch.

          Show
          stack added a comment - @Kannan Yes. Sounds good. Was saying that the forwarded ported RowMutation should be deprecated on forward-port since it does what Mutation is in TRUNK – but we can that in another, subsequent patch.
          Hide
          Lars Hofhansl added a comment -

          @Kannan: Yes, +1

          Show
          Lars Hofhansl added a comment - @Kannan: Yes, +1
          Hide
          Kannan Muthukkaruppan added a comment -

          Actually, RowMutations (in plural) doesn't sound good in documentation. For example saying,

          "A RowMutations which contains a Put & Delete.."

          is grammatically broken and either of the following sounds much better:

          "A RowOperation which contains a Put & Delete.."
          or
          "A AtomicRowMutation which contains a Put & Delete.."

          So, can we go use RowOperation or AtomicRowMutation instead?

          Show
          Kannan Muthukkaruppan added a comment - Actually, RowMutations (in plural) doesn't sound good in documentation. For example saying, "A RowMutations which contains a Put & Delete.." is grammatically broken and either of the following sounds much better: "A RowOperation which contains a Put & Delete.." or "A AtomicRowMutation which contains a Put & Delete.." So, can we go use RowOperation or AtomicRowMutation instead?
          Hide
          stack added a comment -

          Ugh, commented over in different issue:

          stack has commented on the revision "HBASE-5413 [jira] Rename RowMutation to RowMutations".
          
           We have the notion of Operation already.  It would seem to cut across rows in that a Scan and a RowMutations is ugly, agree, but it does convey notions of 'many' and 'in a row'.
          
           Regards RowOperation, Get or Put subclass Operation (actually, its subclass http://hbase.apache.org/apidocs/org/apache/hadoop/hbase/client/OperationWithAttributes.html).    RowOperation therefore could work in that it narrows the set of Operations only, the odd thing is that Delete does not inherit from OperationWIthAttribute.  I'd think we should fix this hole in our 'model' if we went with RowOperation.
          
           If I was looking into the client package and saw both RowMutation and RowOperation sitting beside each other, I'd think I'd be confused what to use whereas with RowMutation and RowMutuations... I'd think I'd have some notion how they might be used.
          
           AtomicRowMutation I feel is w/o meaning since all Row mutations are 'atomic' -- or its a bug - so the Atomic qualifier would seem to add nothing.
          
           As Thomas Pan says, just my 2c.
          
          Show
          stack added a comment - Ugh, commented over in different issue: stack has commented on the revision "HBASE-5413 [jira] Rename RowMutation to RowMutations" . We have the notion of Operation already. It would seem to cut across rows in that a Scan and a RowMutations is ugly, agree, but it does convey notions of 'many' and 'in a row'. Regards RowOperation, Get or Put subclass Operation (actually, its subclass http: //hbase.apache.org/apidocs/org/apache/hadoop/hbase/client/OperationWithAttributes.html). RowOperation therefore could work in that it narrows the set of Operations only, the odd thing is that Delete does not inherit from OperationWIthAttribute. I'd think we should fix this hole in our 'model' if we went with RowOperation. If I was looking into the client package and saw both RowMutation and RowOperation sitting beside each other, I'd think I'd be confused what to use whereas with RowMutation and RowMutuations... I'd think I'd have some notion how they might be used. AtomicRowMutation I feel is w/o meaning since all Row mutations are 'atomic' -- or its a bug - so the Atomic qualifier would seem to add nothing. As Thomas Pan says, just my 2c.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #114 (See https://builds.apache.org/job/HBase-TRUNK-security/114/)
          HBASE-3584 Rename RowMutation to RowMutations (Revision 1245792)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutations.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #114 (See https://builds.apache.org/job/HBase-TRUNK-security/114/ ) HBASE-3584 Rename RowMutation to RowMutations (Revision 1245792) Result = FAILURE stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutations.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Hide
          Lars Hofhansl added a comment -

          Something went wrong with this. I still see RowMutation, but it is empty. I suspect that it was not marked as deleted.

          Show
          Lars Hofhansl added a comment - Something went wrong with this. I still see RowMutation, but it is empty. I suspect that it was not marked as deleted.
          Hide
          Lars Hofhansl added a comment -

          Fixed with addendum.

          Show
          Lars Hofhansl added a comment - Fixed with addendum.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #115 (See https://builds.apache.org/job/HBase-TRUNK-security/115/)
          HBASE-3584 Addendum, remove RowMutation (Revision 1245876)

          Result = FAILURE
          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #115 (See https://builds.apache.org/job/HBase-TRUNK-security/115/ ) HBASE-3584 Addendum, remove RowMutation (Revision 1245876) Result = FAILURE larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2665 (See https://builds.apache.org/job/HBase-TRUNK/2665/)
          HBASE-3584 Addendum, remove RowMutation (Revision 1245876)
          HBASE-3584 Rename RowMutation to RowMutations (Revision 1245792)

          Result = FAILURE
          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java

          stack :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutations.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2665 (See https://builds.apache.org/job/HBase-TRUNK/2665/ ) HBASE-3584 Addendum, remove RowMutation (Revision 1245876) HBASE-3584 Rename RowMutation to RowMutations (Revision 1245792) Result = FAILURE larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutations.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java

            People

            • Assignee:
              Lars Hofhansl
              Reporter:
              ryan rawson
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development