HBase
  1. HBase
  2. HBASE-9032

Result.getBytes() returns null if backed by KeyValue array

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.94.9
    • Fix Version/s: 0.94.11
    • Component/s: Client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This applies only to 0.94 (and earlier) branch.

      If the Result object was constructed using either of Result(KeyValue[]) or Result(List<KeyValue>), calling Result.getBytes() returns null instead of the serialized ImmutableBytesWritable object.

      1. HBASE-9032.patch
        1 kB
        Aditya Kishore
      2. HBASE-9032.patch
        1 kB
        Aditya Kishore
      3. HBASE-9032.patch
        3 kB
        Aditya Kishore
      4. HBASE-9032.patch
        3 kB
        Aditya Kishore

        Activity

        Aditya Kishore created issue -
        Aditya Kishore made changes -
        Field Original Value New Value
        Attachment HBASE-9032.patch [ 12593805 ]
        Hide
        Ted Yu added a comment -
        +      } catch (IOException e) { /* should never happen */ }
        

        Add a log in case of catching IOException ?

        If you reverse the condition for if statement, you should be able to save some indentation.

        Show
        Ted Yu added a comment - + } catch (IOException e) { /* should never happen */ } Add a log in case of catching IOException ? If you reverse the condition for if statement, you should be able to save some indentation.
        Hide
        Aditya Kishore added a comment -

        Attached is a simple patch which serializes the KeyValue array into ImmutableBytesWritable if the former is not null while later is.

        Not submitting patch to Hadoop QA since this is not intended for 0.95 or trunk.

        Show
        Aditya Kishore added a comment - Attached is a simple patch which serializes the KeyValue array into ImmutableBytesWritable if the former is not null while later is. Not submitting patch to Hadoop QA since this is not intended for 0.95 or trunk.
        Hide
        Jean-Marc Spaggiari added a comment -

        Same comment as Ted. Apart from that, I'm +1.

        Show
        Jean-Marc Spaggiari added a comment - Same comment as Ted. Apart from that, I'm +1.
        Hide
        Aditya Kishore added a comment -

        Add a log in case of catching IOException ?

        These are the instances where Java's checked exceptions add unnecessary verbosity to the code. The only error/exception that could happen in this clock is OOM since the IO is to memory buffer. Any attempt to log that would be pointless. Beside, this class has been LOG less so far so did not want to do that.

        In any case, would do if you insist

        If you reverse the condition for if statement, you should be able to save some indentation.

        Either that or introduce double return from the function. Was trying to avoid the later. Further, other functions in this class follow similar style.

        Show
        Aditya Kishore added a comment - Add a log in case of catching IOException ? These are the instances where Java's checked exceptions add unnecessary verbosity to the code. The only error/exception that could happen in this clock is OOM since the IO is to memory buffer. Any attempt to log that would be pointless. Beside, this class has been LOG less so far so did not want to do that. In any case, would do if you insist If you reverse the condition for if statement, you should be able to save some indentation. Either that or introduce double return from the function. Was trying to avoid the later. Further, other functions in this class follow similar style.
        Hide
        Jean-Marc Spaggiari added a comment -

        I will say costs nothing to add a LOG here. It will not add any cpu overhead and as you said should never be called. But if any exception occurs for any obscure reason, that might still help to track it?

        Show
        Jean-Marc Spaggiari added a comment - I will say costs nothing to add a LOG here. It will not add any cpu overhead and as you said should never be called. But if any exception occurs for any obscure reason, that might still help to track it?
        Hide
        Aditya Kishore added a comment -

        Would throwing a wrapped RuntimeException make more sense? Will let the application code decide how to handle.

        Show
        Aditya Kishore added a comment - Would throwing a wrapped RuntimeException make more sense? Will let the application code decide how to handle.
        Hide
        Ted Yu added a comment -

        Throwing RuntimeException is fine.

        Show
        Ted Yu added a comment - Throwing RuntimeException is fine.
        Hide
        Jean-Marc Spaggiari added a comment -

        I will say that both at fine for me. With a little + for the RuntimeException. Ted? On your side?

        Show
        Jean-Marc Spaggiari added a comment - I will say that both at fine for me. With a little + for the RuntimeException. Ted? On your side?
        Hide
        Aditya Kishore added a comment -

        Attached is the new patch on account of review comments.

        Thanks Ted and Jean-Marc for reviewing.

        Show
        Aditya Kishore added a comment - Attached is the new patch on account of review comments. Thanks Ted and Jean-Marc for reviewing.
        Aditya Kishore made changes -
        Attachment HBASE-9032.patch [ 12593828 ]
        Lars Hofhansl made changes -
        Fix Version/s 0.94.11 [ 12324741 ]
        Fix Version/s 0.94.10 [ 12324627 ]
        Hide
        Lars Hofhansl added a comment -

        Do you have a specific use case why you need this?
        The bytes object is only used for serialization. One could argue that getBytes() should not be public.

        I'm not saying we should not fix it, but I'm curious as to why you would need it.

        Show
        Lars Hofhansl added a comment - Do you have a specific use case why you need this? The bytes object is only used for serialization. One could argue that getBytes() should not be public. I'm not saying we should not fix it, but I'm curious as to why you would need it.
        Hide
        Aditya Kishore added a comment -

        Lars,

        I agree that the underlying byte array object is implementation detail and should not be public.

        Unfortunately it is public and as a result some of our advanced users have been using it in their applications. I am not privy to the exact use case but it worked for them until it did not and we diagnosed this to be the reason. They are aware that this is going to go away in next major version but it will be some time before their applications will be migrated to the new APIs.

        Thanks for having a look.

        Show
        Aditya Kishore added a comment - Lars, I agree that the underlying byte array object is implementation detail and should not be public. Unfortunately it is public and as a result some of our advanced users have been using it in their applications. I am not privy to the exact use case but it worked for them until it did not and we diagnosed this to be the reason. They are aware that this is going to go away in next major version but it will be some time before their applications will be migrated to the new APIs. Thanks for having a look.
        Hide
        stack added a comment -

        +1 for 0.94

        Show
        stack added a comment - +1 for 0.94
        Hide
        Aditya Kishore added a comment -

        Updating the patch with a test case.

        Show
        Aditya Kishore added a comment - Updating the patch with a test case.
        Aditya Kishore made changes -
        Attachment HBASE-9032.patch [ 12594585 ]
        Hide
        Jean-Marc Spaggiari added a comment -

        Nice to have a test! Thanks Aditya Kishore. However, I will have prefered to user Assert.assertNotNull(r1.getBytes()); instead of "Result r2 = new Result(r1.getBytes());". Because the only exception you are looking for is the NPE, if any other exception occurs you will still mark this test as failed.

        Show
        Jean-Marc Spaggiari added a comment - Nice to have a test! Thanks Aditya Kishore . However, I will have prefered to user Assert.assertNotNull(r1.getBytes()); instead of "Result r2 = new Result(r1.getBytes());". Because the only exception you are looking for is the NPE, if any other exception occurs you will still mark this test as failed.
        Hide
        Aditya Kishore added a comment -

        Jean-Marc,

        With my patch, r1.getBytes() is always going to be not null (unless someone modifies it).

        What I was going for is verification that deserialization is in sync with serialization, i.e. you can create a valid Result object using the returned ImmutableBytesWritable and that both are equal. compareResults() throws exception if both the Result objects are not equal.

        Show
        Aditya Kishore added a comment - Jean-Marc, With my patch, r1.getBytes() is always going to be not null (unless someone modifies it). What I was going for is verification that deserialization is in sync with serialization, i.e. you can create a valid Result object using the returned ImmutableBytesWritable and that both are equal. compareResults() throws exception if both the Result objects are not equal.
        Hide
        Jean-Marc Spaggiari added a comment -

        Ok I see.

        There can be 2 cases here.

        1) r1.getBytes() can be null (before your patch).
        2) r1.getBytes() can be different than expected.

        If r1.getBytes() send null, Result() constructor will still work but the comparison will fail. If r1.getBytes() works but send a differenlt result that expected, the comparison will faile too. So you don't really have a differentiation between the 2 cases. Adding Assert.assertNotNull(r1.getBytes()) will allow to differentiate that. In both cases, test will fail, but that will help to know what failed in it.

        So, forget what I said about " instead of Result r2 = new", it should have been " in addition of Result.compareResults(r1, r2);" (just before).

        Make sense?

        Show
        Jean-Marc Spaggiari added a comment - Ok I see. There can be 2 cases here. 1) r1.getBytes() can be null (before your patch). 2) r1.getBytes() can be different than expected. If r1.getBytes() send null, Result() constructor will still work but the comparison will fail. If r1.getBytes() works but send a differenlt result that expected, the comparison will faile too. So you don't really have a differentiation between the 2 cases. Adding Assert.assertNotNull(r1.getBytes()) will allow to differentiate that. In both cases, test will fail, but that will help to know what failed in it. So, forget what I said about " instead of Result r2 = new", it should have been " in addition of Result.compareResults(r1, r2);" (just before). Make sense?
        Hide
        Aditya Kishore added a comment -

        Sure. Patch modified and attached.

        Show
        Aditya Kishore added a comment - Sure. Patch modified and attached.
        Aditya Kishore made changes -
        Attachment HBASE-9032.patch [ 12594617 ]
        Hide
        Jean-Marc Spaggiari added a comment -

        Excellent. Thanks Aditya Kishore! I'm +1.

        You have +1 from Stack, so I guess you just need another +1 from another commiter to get it pushed. Lars Hofhansl, good for you?

        Show
        Jean-Marc Spaggiari added a comment - Excellent. Thanks Aditya Kishore ! I'm +1. You have +1 from Stack, so I guess you just need another +1 from another commiter to get it pushed. Lars Hofhansl , good for you?
        Hide
        Lars Hofhansl added a comment -

        Sorry, -0 from me.

        1. This ties the caller to the internal workings of the Result object
        2. It cements this API even further for other user just starting with 0.94. This method should have never been public.
        3. As you point out, this API is going away in 0.95
        4. There might be code out there relying on the current behavior (maybe for detecting whether a Result object was deserialized from RPC)

        Now, I am not blocking it. It won't do much harm I think. But I would prefer if somebody else commits it. stack?

        Show
        Lars Hofhansl added a comment - Sorry, -0 from me. This ties the caller to the internal workings of the Result object It cements this API even further for other user just starting with 0.94. This method should have never been public. As you point out, this API is going away in 0.95 There might be code out there relying on the current behavior (maybe for detecting whether a Result object was deserialized from RPC) Now, I am not blocking it. It won't do much harm I think. But I would prefer if somebody else commits it. stack ?
        Hide
        stack added a comment -

        I like Lars Hofhansl take that this should never have been public. Given that it is and that it sometimes is broke, I committed the patch to 0.94 trunk. Thanks Aditya.

        Show
        stack added a comment - I like Lars Hofhansl take that this should never have been public. Given that it is and that it sometimes is broke, I committed the patch to 0.94 trunk. Thanks Aditya.
        stack made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Hadoop Flags Reviewed [ 10343 ]
        Resolution Fixed [ 1 ]
        Hide
        Aditya Kishore added a comment -

        Thanks Lars, Stack.

        Show
        Aditya Kishore added a comment - Thanks Lars, Stack.
        Lars Hofhansl made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Aditya Kishore
            Reporter:
            Aditya Kishore
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development