HBase
  1. HBase
  2. HBASE-6009

Changes for HBASE-5209 are technically incompatible

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 0.92.1, 0.94.0
    • Fix Version/s: None
    • Component/s: master
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      HBASE-5209 was introduced in 0.92.1 and 0.94.0 but introduced an incompatibility from a 0.92.0 client. Since this had maded it into two releases already, we've decided to leave it in.
      Show
      HBASE-5209 was introduced in 0.92.1 and 0.94.0 but introduced an incompatibility from a 0.92.0 client. Since this had maded it into two releases already, we've decided to leave it in.

      Description

      The additions to add backup masters to ClusterStatus are technically incompatible between clients and servers. Older clients will basically not read the extra bits that the newer server pushes for the backup masters, thus screwing up the serialization for the next blob in the pipe.

      For the Writable, we can add a total size field for ClusterStatus at the beginning, or we can have start and end markers. I can make a patch for either approach; interested in whatever folks have to suggest. Would be good to get this in soon to limit the damage to 0.92.1 (don't know if we can get this in in time for 0.94.0).

      Either change will make us forward-compatible starting with when the change goes in, but will not fix the backwards incompatibility, which we will have to mark with a release note as there have already been releases with this change.

      Hopefully we can do this in a cleaner way when wire compat rolls around in 0.96.

        Activity

        Hide
        Ted Yu added a comment -

        or we can have start and end markers

        How do we define the markers ? Through a series of magic bytes ?

        Looks like total size field for ClusterStatus is better choice.

        Show
        Ted Yu added a comment - or we can have start and end markers How do we define the markers ? Through a series of magic bytes ? Looks like total size field for ClusterStatus is better choice.
        Hide
        David S. Wang added a comment -

        I looked at the total size field option for this, starting from the write case.

        To calculate total size written, you have to know how many bytes were written for each write() call on ClusterStatus, including any objects contained inside it.

        The DataOutput interface for Writables doesn't have a way to return how many bytes were written to the stream. This is not a problem for primitive types as we can figure that out trivially. Even for somewhat more complicated situations such as modified UTF-8s written with the writeUTF call, the number of written bytes for a String can at least be calculated based on the formula for modified UTF-8 conversion.

        However, for calls to Object's write functions (e.g. for HRegionLoad), this becomes somewhat more problematic as there is no obvious answer as to how many bytes were written. We could use reflection to grab the fields, but then there is no guarantee that all of the fields of the Object are actually written to the stream when write() is called. So you'd have to introduce some hardcoded way of knowing how much was written for each Object, which is Bad.

        I'm tempted to say that we shouldn't add any more fields to ClusterStatus or similar APIs until 0.96, when hopefully our wire compatibility efforts will kick in and we can do this in a compatible way without having to jump through hoops.

        Show
        David S. Wang added a comment - I looked at the total size field option for this, starting from the write case. To calculate total size written, you have to know how many bytes were written for each write() call on ClusterStatus, including any objects contained inside it. The DataOutput interface for Writables doesn't have a way to return how many bytes were written to the stream. This is not a problem for primitive types as we can figure that out trivially. Even for somewhat more complicated situations such as modified UTF-8s written with the writeUTF call, the number of written bytes for a String can at least be calculated based on the formula for modified UTF-8 conversion. However, for calls to Object's write functions (e.g. for HRegionLoad), this becomes somewhat more problematic as there is no obvious answer as to how many bytes were written. We could use reflection to grab the fields, but then there is no guarantee that all of the fields of the Object are actually written to the stream when write() is called. So you'd have to introduce some hardcoded way of knowing how much was written for each Object, which is Bad. I'm tempted to say that we shouldn't add any more fields to ClusterStatus or similar APIs until 0.96, when hopefully our wire compatibility efforts will kick in and we can do this in a compatible way without having to jump through hoops.
        Show
        Ted Yu added a comment - @David: See the following: http://stackoverflow.com/questions/2984538/how-to-use-bytearrayoutputstream-and-dataoutputstream-simultaneously-java
        Hide
        stack added a comment -

        Lets try and avoid serializing fat ClusterStatus objects twice.

        We have never made guarantee that old clients could talk to new servers pre-0.96 (Too hard in the Writables world). What is the scenario David? You cannot update the clients? Wouldn't you have to update the clients anyways if you introduce clusterstatus size? Pardon me if I'm not getting this.

        Show
        stack added a comment - Lets try and avoid serializing fat ClusterStatus objects twice. We have never made guarantee that old clients could talk to new servers pre-0.96 (Too hard in the Writables world). What is the scenario David? You cannot update the clients? Wouldn't you have to update the clients anyways if you introduce clusterstatus size? Pardon me if I'm not getting this.
        Hide
        David S. Wang added a comment -

        The immediate issue here is that HBASE-5209 was committed in 0.92.1, and that broke compatibility with 0.92.0. I suppose anyone who cares about 0.92 branch has moved to 0.92.1 so that there is no practical hit.

        You are right that adding a size would be another incompatible change, hence my later comment about "let's just not make any more changes until 0.96".

        Anyway in the absence of any changes, I can at least add a release note to 0.92.1 stating this incompatibility with 0.92.0. I'll use this JIRA to track that.

        Show
        David S. Wang added a comment - The immediate issue here is that HBASE-5209 was committed in 0.92.1, and that broke compatibility with 0.92.0. I suppose anyone who cares about 0.92 branch has moved to 0.92.1 so that there is no practical hit. You are right that adding a size would be another incompatible change, hence my later comment about "let's just not make any more changes until 0.96". Anyway in the absence of any changes, I can at least add a release note to 0.92.1 stating this incompatibility with 0.92.0. I'll use this JIRA to track that.
        Hide
        Enis Soztutar added a comment -

        +1 for the release note on 0.92.1.

        Show
        Enis Soztutar added a comment - +1 for the release note on 0.92.1.
        Hide
        stack added a comment -

        bq ...that HBASE-5209 was committed in 0.92.1, and that broke compatibility with 0.92.0

        That was dumb. Did I do it? Probably.

        Should we back it out for 0.92.2?

        Show
        stack added a comment - bq ...that HBASE-5209 was committed in 0.92.1, and that broke compatibility with 0.92.0 That was dumb. Did I do it? Probably. Should we back it out for 0.92.2?
        Hide
        Ted Yu added a comment -

        HBASE-5209 is an improvement.
        Backing it out wouldn't be too bad.

        Show
        Ted Yu added a comment - HBASE-5209 is an improvement. Backing it out wouldn't be too bad.
        Hide
        David S. Wang added a comment -

        I wouldn't back it out ... I think that would just create another incompatibility event between 0.92.1 and 0.92.2.

        The original goal was to get this committed for 0.92.0 (which would have avoided this), but I suppose we didn't make it for 0.92.0. I should have said something about not doing this once 0.92.0 came out, so my apologies on that.

        Show
        David S. Wang added a comment - I wouldn't back it out ... I think that would just create another incompatibility event between 0.92.1 and 0.92.2. The original goal was to get this committed for 0.92.0 (which would have avoided this), but I suppose we didn't make it for 0.92.0. I should have said something about not doing this once 0.92.0 came out, so my apologies on that.
        Hide
        Jonathan Hsieh added a comment -

        It seems like from discussion and inaction, we've decided to keep this in the acknowledge the incompatiblity between 0.92.0 and 0.92.1+ series, and keep this in 0.94.0 series allowing for compatibility between 0.92.1+ and 0.94.0

        Show
        Jonathan Hsieh added a comment - It seems like from discussion and inaction, we've decided to keep this in the acknowledge the incompatiblity between 0.92.0 and 0.92.1+ series, and keep this in 0.94.0 series allowing for compatibility between 0.92.1+ and 0.94.0

          People

          • Assignee:
            David S. Wang
            Reporter:
            David S. Wang
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development