HBase
  1. HBase
  2. HBASE-9834

Minimize byte[] copies for 'smart' clients

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.14
    • Component/s: Client
    • Labels:
      None
    • Release Note:
      Hide
      Committed v4 -thanks for the reviews.

      Roll away Lars.
      Show
      Committed v4 -thanks for the reviews. Roll away Lars.
    • Tags:
      Phoenix

      Description

      'Smart' clients (e.g. phoenix) that have in-depth knowledge of HBase often bemoan the extra byte[] copies that must be done when building multiple puts/deletes. We should provide a mechanism by which they can minimize these copies, but still remain wire compatible.

      1. hbase-9834-0.94-v0.patch
        25 kB
        Jesse Yates
      2. hbase-9834-0.94-v1.patch
        27 kB
        Jesse Yates
      3. hbase-9834-0.94-v2.patch
        33 kB
        Jesse Yates
      4. hbase-9834-0.94-v3.patch
        11 kB
        Jesse Yates
      5. hbase-9834-0.94-v4.patch
        5 kB
        Jesse Yates

        Issue Links

          Activity

          Hide
          Jesse Yates added a comment -

          Patch coming momentarily for 0.94 (easier to grok for the moment). 0.96/98 shouldn't be too bad, though haven't looked yet .

          Show
          Jesse Yates added a comment - Patch coming momentarily for 0.94 (easier to grok for the moment). 0.96/98 shouldn't be too bad, though haven't looked yet .
          Hide
          Jesse Yates added a comment -

          Attaching 0.94 version. Turns out, we can also re-use this for our own clients. Should help cut down some of the time it takes to build Put/Deletes since we don't need to copy all the bytes in each time we create a new kv.

          Show
          Jesse Yates added a comment - Attaching 0.94 version. Turns out, we can also re-use this for our own clients. Should help cut down some of the time it takes to build Put/Deletes since we don't need to copy all the bytes in each time we create a new kv.
          Hide
          Jesse Yates added a comment -

          Also, shout out to Lars Hofhansl and James Taylor for inspiration.

          Show
          Jesse Yates added a comment - Also, shout out to Lars Hofhansl and James Taylor for inspiration.
          Hide
          Nick Dimiduk added a comment -

          What's the thinking behind using ImmutableBytesWritable instead of ByteBuffer or ByteRange?

          Show
          Nick Dimiduk added a comment - What's the thinking behind using ImmutableBytesWritable instead of ByteBuffer or ByteRange?
          Hide
          Jesse Yates added a comment -

          Oops, missed a couple tests/null checks. Here is an updated verison that fixes a couple of the matchesXXX (+ tests!).

          Show
          Jesse Yates added a comment - Oops, missed a couple tests/null checks. Here is an updated verison that fixes a couple of the matchesXXX (+ tests!).
          Hide
          Jesse Yates added a comment -

          KeyValues are immutable, so immutable bytes writable seemed a reasonable use. Also, I find ByteBuffer much harder to manage - we don't need all the flipping/setting/removing stuff. Also, in 0.94 we don't have ByteRange (but the immutable logic still holds from before).

          Also, Phoenix uses ImmutableBytesWritable (or rather a subclass) everywhere, so dovetails nicely with their impl.

          Show
          Jesse Yates added a comment - KeyValues are immutable, so immutable bytes writable seemed a reasonable use. Also, I find ByteBuffer much harder to manage - we don't need all the flipping/setting/removing stuff. Also, in 0.94 we don't have ByteRange (but the immutable logic still holds from before). Also, Phoenix uses ImmutableBytesWritable (or rather a subclass) everywhere, so dovetails nicely with their impl.
          Hide
          Lars Hofhansl added a comment -

          Will all the public static methods in KeyValue still work (like all the compare functions)?
          Will Put's/Mutatin's get(), get/setFamilyMap, etc, still work?
          Clients that use these will see a different behavior if they use Puts/Deletes to create KeyValues for them. Not a likely use case, but possible.

          Lastly, this does not strictly need to be part of HBase as such, right? A client could build up an object that serializes like a Put similar to how we have an object that serializes like a KeyValue here.

          I'm all for having ClientKeyValue in HBase (so that memory conscious clients can use it), but the changes to Put and Delete have to be thought through carefully.

          Show
          Lars Hofhansl added a comment - Will all the public static methods in KeyValue still work (like all the compare functions)? Will Put's/Mutatin's get(), get/setFamilyMap, etc, still work? Clients that use these will see a different behavior if they use Puts/Deletes to create KeyValues for them. Not a likely use case, but possible. Lastly, this does not strictly need to be part of HBase as such, right? A client could build up an object that serializes like a Put similar to how we have an object that serializes like a KeyValue here. I'm all for having ClientKeyValue in HBase (so that memory conscious clients can use it), but the changes to Put and Delete have to be thought through carefully.
          Hide
          Jesse Yates added a comment -

          Yeah, those are reasonable concerns Lars. I was thinking...maybe?

          For this to work even without that we just need to change how Put writes KVs (which is different than how Delete does it - KV#write - and instead has the same logic copied over (legacy?)).

          Alternative would be to fix up the last couple methods in ClientKeyValue (e.g. split, getBuffer) to match KeyValue and drop this is implementation into KeyValue instead. Could play some tricks to make it about as fast (caching the full buffer when we request it, etc) Really, this seems better for how KV should work anyways.... maybe a bit overkill

          Thoughts?

          Show
          Jesse Yates added a comment - Yeah, those are reasonable concerns Lars. I was thinking...maybe? For this to work even without that we just need to change how Put writes KVs (which is different than how Delete does it - KV#write - and instead has the same logic copied over (legacy?)). Alternative would be to fix up the last couple methods in ClientKeyValue (e.g. split, getBuffer) to match KeyValue and drop this is implementation into KeyValue instead. Could play some tricks to make it about as fast (caching the full buffer when we request it, etc) Really, this seems better for how KV should work anyways.... maybe a bit overkill Thoughts?
          Hide
          Jesse Yates added a comment - - edited

          Attaching updated 0.94 patch. This is the simple, non-invasive update that keeps with the original "just add support" approach, rather than completely re-writing keyvalue.

          This patch removes the default to ClientKeyValue in Put/Delete creation (so everything is again just a KeyValue by default) from the last patch

          Other notable changes were necessary though in testing (added a few more) to allow a ClientKeyValue to work. For instance, Delete#addDeleteMarker only supported adding KeyValue.Type.Delete, but really should support all the delete types. Also, needed to fix the row comparison exception since it expects KeyValue to be backed by a buffer when printing the error message.

          Show
          Jesse Yates added a comment - - edited Attaching updated 0.94 patch. This is the simple, non-invasive update that keeps with the original "just add support" approach, rather than completely re-writing keyvalue. This patch removes the default to ClientKeyValue in Put/Delete creation (so everything is again just a KeyValue by default) from the last patch Other notable changes were necessary though in testing (added a few more) to allow a ClientKeyValue to work. For instance, Delete#addDeleteMarker only supported adding KeyValue.Type.Delete, but really should support all the delete types. Also, needed to fix the row comparison exception since it expects KeyValue to be backed by a buffer when printing the error message.
          Hide
          Jesse Yates added a comment -

          Other thing I'd like to add is to actually be able to pass in an ImmutableBytesWritable to a mutation for the row, so we could completely reuse the bytes everywhere. Might fit here, but thinking about just doing in a follow up.

          Show
          Jesse Yates added a comment - Other thing I'd like to add is to actually be able to pass in an ImmutableBytesWritable to a mutation for the row, so we could completely reuse the bytes everywhere. Might fit here, but thinking about just doing in a follow up.
          Hide
          Andrew Purtell added a comment -

          Patch v2 looks innocuous. I'm not sure if ClientKeyValue provides enough value to balance out being a source of confusion. What if we just commit the changes to Delete and Put that make this possible? Overall +0 I guess.

          Show
          Andrew Purtell added a comment - Patch v2 looks innocuous. I'm not sure if ClientKeyValue provides enough value to balance out being a source of confusion. What if we just commit the changes to Delete and Put that make this possible? Overall +0 I guess.
          Hide
          Jesse Yates added a comment -

          I like that idea Andy - changes to Put/Delete are small enough no one should mind. I'll drop the ClientKeyValue into the phoenix source

          Show
          Jesse Yates added a comment - I like that idea Andy - changes to Put/Delete are small enough no one should mind. I'll drop the ClientKeyValue into the phoenix source
          Hide
          Lars Hofhansl added a comment -

          +1 to just having the changes to Put/Delete.

          Show
          Lars Hofhansl added a comment - +1 to just having the changes to Put/Delete.
          Hide
          Lars Hofhansl added a comment -

          Wanna just commit the Delete/Put changes? Are there more changes in any of the other subclasses of Mutation needed?

          Show
          Lars Hofhansl added a comment - Wanna just commit the Delete/Put changes? Are there more changes in any of the other subclasses of Mutation needed?
          Hide
          Jesse Yates added a comment -

          Attaching patch for 0.94 (trunk coming soon). Moves the family map writing into Mutation as an accessible method so we don't do this code copying everywhere (it was 3x - Put, Delete, Append).

          Also, adding an accessor method in Append to make this usable - it gets #add(KeyValue) in addition to its existing #add(byte[], byte[], byte[]), similar to how Delete works.

          Show
          Jesse Yates added a comment - Attaching patch for 0.94 (trunk coming soon). Moves the family map writing into Mutation as an accessible method so we don't do this code copying everywhere (it was 3x - Put, Delete, Append). Also, adding an accessor method in Append to make this usable - it gets #add(KeyValue) in addition to its existing #add(byte[], byte[], byte[]), similar to how Delete works.
          Hide
          Jesse Yates added a comment -

          Looks like trunk/0.96 stuff doesn't need anything special because the move to cell doesn't make the same assumptions about a backing buffer and just goes through interfaces.

          Lars Hofhansl, what do you think about v3 for 0.94?

          Show
          Jesse Yates added a comment - Looks like trunk/0.96 stuff doesn't need anything special because the move to cell doesn't make the same assumptions about a backing buffer and just goes through interfaces. Lars Hofhansl , what do you think about v3 for 0.94?
          Hide
          Lars Hofhansl added a comment -

          +1
          Nice cleanup.

          Show
          Lars Hofhansl added a comment - +1 Nice cleanup.
          Hide
          Jesse Yates added a comment -

          Cool. Committing this afternoon (PST) unless there are any objections

          Show
          Jesse Yates added a comment - Cool. Committing this afternoon (PST) unless there are any objections
          Hide
          Jesse Yates added a comment -

          Committed to 0.94. thanks for the reviews all.

          Show
          Jesse Yates added a comment - Committed to 0.94. thanks for the reviews all.
          Hide
          stack added a comment -

          +1 Thanks for removing dup'd code.

          Show
          stack added a comment - +1 Thanks for removing dup'd code.
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-0.94-security #336 (See https://builds.apache.org/job/HBase-0.94-security/336/)
          HBASE-9834: Minimize byte[] copies for 'smart' clients (jyates: rev 1542052)

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Mutation.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-0.94-security #336 (See https://builds.apache.org/job/HBase-0.94-security/336/ ) HBASE-9834 : Minimize byte[] copies for 'smart' clients (jyates: rev 1542052) /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Mutation.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-0.94 #1202 (See https://builds.apache.org/job/HBase-0.94/1202/)
          HBASE-9834: Minimize byte[] copies for 'smart' clients (jyates: rev 1542052)

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Mutation.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-0.94 #1202 (See https://builds.apache.org/job/HBase-0.94/1202/ ) HBASE-9834 : Minimize byte[] copies for 'smart' clients (jyates: rev 1542052) /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Mutation.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Hide
          Lars Hofhansl added a comment -

          I think this breaks wire-compatibility. Might need to roll this back.

          Show
          Lars Hofhansl added a comment - I think this breaks wire-compatibility. Might need to roll this back.
          Hide
          Andrew Purtell added a comment - - edited

          Looks like a 1 line error in writeFamilyMap?

          Edit: Never mind. An amendment or revert and commit of an update that restores the differences in how Delete writes out the family map versus others could fix the issue.

          Show
          Andrew Purtell added a comment - - edited Looks like a 1 line error in writeFamilyMap? Edit: Never mind. An amendment or revert and commit of an update that restores the differences in how Delete writes out the family map versus others could fix the issue.
          Hide
          Lars Hofhansl added a comment -

          Sigh... Turns out a Delete's familyMap is encoded differently from a Put's familyMap, even though it is the same thing W.T.F.

          Anyway, I'll revert this for now, and then we can regroup.

          Show
          Lars Hofhansl added a comment - Sigh... Turns out a Delete's familyMap is encoded differently from a Put's familyMap, even though it is the same thing W.T.F. Anyway, I'll revert this for now, and then we can regroup.
          Hide
          Andrew Purtell added a comment -

          Yeah see above edit.

          Show
          Andrew Purtell added a comment - Yeah see above edit.
          Hide
          Lars Hofhansl added a comment -

          Yep. Thanks Andy, you beat me
          Reverted for now.

          I now wonder how we can write a test to catch these.

          Maybe generate a file will all kinds of a objects, save that. Then

          1. read that back
          2. regenerate and make sure it's identical

          that way we ensured that neither readFields nor write has changed.

          Show
          Lars Hofhansl added a comment - Yep. Thanks Andy, you beat me Reverted for now. I now wonder how we can write a test to catch these. Maybe generate a file will all kinds of a objects, save that. Then read that back regenerate and make sure it's identical that way we ensured that neither readFields nor write has changed.
          Hide
          Andrew Purtell added a comment -

          I now wonder how we can write a test to catch these.
          Maybe generate a file will all kinds of a objects, save that. Then
          read that back
          regenerate and make sure it's identical
          that way we ensured that neither readFields nor write has changed.

          Ok, HBASE-9981

          Show
          Andrew Purtell added a comment - I now wonder how we can write a test to catch these. Maybe generate a file will all kinds of a objects, save that. Then read that back regenerate and make sure it's identical that way we ensured that neither readFields nor write has changed. Ok, HBASE-9981
          Hide
          Andrew Purtell added a comment -

          Closed HBASE-9981 as dup of HBASE-9980.

          Show
          Andrew Purtell added a comment - Closed HBASE-9981 as dup of HBASE-9980 .
          Hide
          Jesse Yates added a comment -

          Thanks for the catch fellas

          Attaching updated version that just the original intentions:

          • fixing the write out of each KV for Put and Append to just use the keyvalue#write (which maintains write compat since it does the exact same thing)
          • uses the KV#getRow comparison rather than assuming byte[] backing
          • adds support for adding kvs to Append
          • add support for a wider range of Delete KV markers (before was just DELETE type, now also DELTE_FAMILY and DELETE_COLUMN).

          no more trying to get too smart

          Show
          Jesse Yates added a comment - Thanks for the catch fellas Attaching updated version that just the original intentions: fixing the write out of each KV for Put and Append to just use the keyvalue#write (which maintains write compat since it does the exact same thing) uses the KV#getRow comparison rather than assuming byte[] backing adds support for adding kvs to Append add support for a wider range of Delete KV markers (before was just DELETE type, now also DELTE_FAMILY and DELETE_COLUMN). no more trying to get too smart
          Hide
          Lars Hofhansl added a comment -

          +1 on V4.

          Show
          Lars Hofhansl added a comment - +1 on V4.
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-0.94 #1203 (See https://builds.apache.org/job/HBase-0.94/1203/)
          HBASE-9834 Revert, due to wire compatibility issues (larsh: rev 1542440)

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Mutation.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-0.94 #1203 (See https://builds.apache.org/job/HBase-0.94/1203/ ) HBASE-9834 Revert, due to wire compatibility issues (larsh: rev 1542440) /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Mutation.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in HBase-0.94-security #337 (See https://builds.apache.org/job/HBase-0.94-security/337/)
          HBASE-9834 Revert, due to wire compatibility issues (larsh: rev 1542440)

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Mutation.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Show
          Hudson added a comment - SUCCESS: Integrated in HBase-0.94-security #337 (See https://builds.apache.org/job/HBase-0.94-security/337/ ) HBASE-9834 Revert, due to wire compatibility issues (larsh: rev 1542440) /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Mutation.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Hide
          Lars Hofhansl added a comment -

          Jesse Yates, feel free to commit, so that I can roll 0.94.14RC0.

          Show
          Lars Hofhansl added a comment - Jesse Yates , feel free to commit, so that I can roll 0.94.14RC0.
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in HBase-0.94-security #338 (See https://builds.apache.org/job/HBase-0.94-security/338/)
          HBASE-9834: Minimize byte[] copies for 'smart' clients (again) (jyates: rev 1542645)

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Show
          Hudson added a comment - SUCCESS: Integrated in HBase-0.94-security #338 (See https://builds.apache.org/job/HBase-0.94-security/338/ ) HBASE-9834 : Minimize byte[] copies for 'smart' clients (again) (jyates: rev 1542645) /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in HBase-0.94 #1204 (See https://builds.apache.org/job/HBase-0.94/1204/)
          HBASE-9834: Minimize byte[] copies for 'smart' clients (again) (jyates: rev 1542645)

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java
          Show
          Hudson added a comment - SUCCESS: Integrated in HBase-0.94 #1204 (See https://builds.apache.org/job/HBase-0.94/1204/ ) HBASE-9834 : Minimize byte[] copies for 'smart' clients (again) (jyates: rev 1542645) /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Append.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Delete.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/Put.java

            People

            • Assignee:
              Jesse Yates
              Reporter:
              Jesse Yates
            • Votes:
              0 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development