HBase
  1. HBase
  2. HBASE-5204

Backward compatibility fixes for 0.92

    Details

    • Hadoop Flags:
      Incompatible change, Reviewed

      Description

      Attached are 3 patches that are necessary to allow compatibility between HBase 0.90.x (and previous releases) and HBase 0.92.0.

      First of all, I'm well aware that 0.92.0 RC4 has been thumbed up by a lot of people and would probably wind up being released as 0.92.0 tomorrow, so I sincerely apologize for creating this issue so late in the process. I spent a lot of time trying to work around the quirks of 0.92 but once I realized that with a few very quasi-trivial changes compatibility would be made significantly easier, I immediately sent these 3 patches to Stack, who suggested I create this issue.

      The first patch is required as without it clients sending a 0.90-style RPC to a 0.92-style server causes the server to die uncleanly. It seems that 0.92 ships with -XX:OnOutOfMemoryError="kill -9 %p", and when a 0.92 server fails to deserialize a 0.90-style RPC, it attempts to allocate a large buffer because it doesn't read fields of 0.90-style RPCs properly. This allocation attempt immediately triggers an OOME, which causes the JVM to die abruptly of a SIGKILL. So whenever a 0.90.x client attempts to connect to HBase, it kills whichever RS is hosting the -ROOT- region.

      The second patch fixes a bug introduced by HBASE-2002, which added support for letting clients specify what "protocol" they want to speak. If a client doesn't properly specify what protocol to use, the connection's protocol field will be left null, which causes any subsequent RPC on that connection to trigger an NPE in the server, even though the connection was successfully established from the client's point of view. The fix is to simply give the connection a default protocol, by assuming the client meant to speak to a RegionServer.

      The third patch fixes an oversight that slipped in HBASE-451, where a change to HbaseObjectWritable caused all the codes used to serialize Writables to shift by one. This was carefully avoided in other changes such as HBASE-1502, which cleanly removed entries for HMsg and HMsg[], so I don't think this breakage in HBASE-451 was intended.

      1. 0001-Add-some-backward-compatible-support-for-reading-old.patch
        3 kB
        Benoit Sigoure
      2. 0002-Make-sure-that-a-connection-always-uses-a-protocol.patch
        2 kB
        Benoit Sigoure
      3. 0003-Change-the-code-used-when-serializing-HTableDescript.patch
        3 kB
        Benoit Sigoure
      4. 5204.addendum
        1 kB
        Ted Yu
      5. 5204-92.txt
        5 kB
        Ted Yu
      6. 5204-trunk.txt
        5 kB
        Ted Yu

        Issue Links

          Activity

          Hide
          stack added a comment -

          Yes, agree, adding patch to trunk would be silly. Lets drop it. The codes should never have been changed. Hopefully your warning...

          1231985      stack     ////////////////////////////////////////////////////////////////////////////
          1231985      stack     // WARNING: Please do not insert, remove or swap any line in this static  //
          1231985      stack     // block.  Doing so would change or shift all the codes used to serialize //
          1231985      stack     // objects, which makes backwards compatibility very hard for clients.    //
          1231985      stack     // New codes should always be added at the end. Code removal is           //
          1231985      stack     // discouraged because code is a short now.                               //
          1231985      stack     ////////////////////////////////////////////////////////////////////////////
          

          .... will help w/ that.

          Can we check in an asynchbase unit test that exercises all apis you need so we fail fast in case we mess up again (HBase has 16 committers now and hard to have them all on message)

          Show
          stack added a comment - Yes, agree, adding patch to trunk would be silly. Lets drop it. The codes should never have been changed. Hopefully your warning... 1231985 stack //////////////////////////////////////////////////////////////////////////// 1231985 stack // WARNING: Please do not insert, remove or swap any line in this static // 1231985 stack // block. Doing so would change or shift all the codes used to serialize // 1231985 stack // objects, which makes backwards compatibility very hard for clients. // 1231985 stack // New codes should always be added at the end. Code removal is // 1231985 stack // discouraged because code is a short now. // 1231985 stack //////////////////////////////////////////////////////////////////////////// .... will help w/ that. Can we check in an asynchbase unit test that exercises all apis you need so we fail fast in case we mess up again (HBase has 16 committers now and hard to have them all on message)
          Hide
          Benoit Sigoure added a comment -

          I agree with Zhihong. Either this makes it into 0.92, or we just forget about it.

          I talked to Stack and tried to argue that including this completely harmless change in 0.92 wouldn't require a new RC. But apparently there's an Apache rule that you can't change a single byte of the code without going through a new RC, so I gave up on the idea.

          Including this change in trunk will cause confusion for future versions and will make my asynchbase's life even more miserable than it is. Yes Stack in theory I can rejigger the codes based on what version of HBase I'm talking to, but in practice this I'd rather not do this.

          It was a mistake to break the codes in the first place. Either this mistake gets fixed right now, in 0.92.0, or we just accept it and live with it and promise not to break them again in the future.

          Show
          Benoit Sigoure added a comment - I agree with Zhihong. Either this makes it into 0.92, or we just forget about it. I talked to Stack and tried to argue that including this completely harmless change in 0.92 wouldn't require a new RC. But apparently there's an Apache rule that you can't change a single byte of the code without going through a new RC, so I gave up on the idea. Including this change in trunk will cause confusion for future versions and will make my asynchbase's life even more miserable than it is. Yes Stack in theory I can rejigger the codes based on what version of HBase I'm talking to, but in practice this I'd rather not do this. It was a mistake to break the codes in the first place. Either this mistake gets fixed right now, in 0.92.0, or we just accept it and live with it and promise not to break them again in the future.
          Hide
          Ted Yu added a comment -

          Applying the addendum to TRUNK only would make codes for classes out of sync among 0.90, 0.92 and 0.94
          Confusion would be higher.

          Show
          Ted Yu added a comment - Applying the addendum to TRUNK only would make codes for classes out of sync among 0.90, 0.92 and 0.94 Confusion would be higher.
          Hide
          stack added a comment -

          And, this issue can't go into 0.92 because it will break rolling restart (for those accessing codes that have been reordered). Can go into trunk.

          Show
          stack added a comment - And, this issue can't go into 0.92 because it will break rolling restart (for those accessing codes that have been reordered). Can go into trunk.
          Hide
          stack added a comment -

          Offline I talked to Benoit. Changing order is a nice to have. This issue is not an RC blocker.

          Show
          stack added a comment - Offline I talked to Benoit. Changing order is a nice to have. This issue is not an RC blocker.
          Hide
          stack added a comment -

          Zhihong Would suggest you wait on Benoît feedback.

          This means a new RC? Thats kinda obscene. In your client B, can you not rejigger the codes dependent on hbase version?

          Show
          stack added a comment - Zhihong Would suggest you wait on Benoît feedback. This means a new RC? Thats kinda obscene. In your client B, can you not rejigger the codes dependent on hbase version?
          Hide
          Ted Yu added a comment -

          Will integrate addendum in one hour if there is no objection.

          Show
          Ted Yu added a comment - Will integrate addendum in one hour if there is no objection.
          Hide
          Ted Yu added a comment -

          Here is the diff after addendum is applied:

          zhihyu$ diff ../90hbase/obj-writable.90 obj-writable.92
          20,21c20,21
          < 20     addToMap(HMsg.class, code++);
          < 21     addToMap(HMsg[].class, code++);
          ---
          > 20     addToMap(Integer.class, code++);
          > 21     addToMap(Integer[].class, code++);
          57,58c57,58
          < 57     addToMap(MultiPut.class, code++);
          < 58     addToMap(MultiPutResponse.class, code++);
          ---
          > 57     addToMap(BitComparator.class, code++);
          > 58     addToMap(Exec.class, code++);
          71a72,77
          > 72     addToMap(RandomRowFilter.class, code++);
          > 73     addToMap(CompareOp.class, code++);
          > 74     addToMap(ColumnRangeFilter.class, code++);
          > 75     addToMap(HServerLoad.class, code++);
          > 76     addToMap(RegionOpeningState.class, code++);
          > 77     addToMap(HTableDescriptor[].class, code++);
          
          Show
          Ted Yu added a comment - Here is the diff after addendum is applied: zhihyu$ diff ../90hbase/obj-writable.90 obj-writable.92 20,21c20,21 < 20 addToMap(HMsg.class, code++); < 21 addToMap(HMsg[].class, code++); --- > 20 addToMap( Integer .class, code++); > 21 addToMap( Integer [].class, code++); 57,58c57,58 < 57 addToMap(MultiPut.class, code++); < 58 addToMap(MultiPutResponse.class, code++); --- > 57 addToMap(BitComparator.class, code++); > 58 addToMap(Exec.class, code++); 71a72,77 > 72 addToMap(RandomRowFilter.class, code++); > 73 addToMap(CompareOp.class, code++); > 74 addToMap(ColumnRangeFilter.class, code++); > 75 addToMap(HServerLoad.class, code++); > 76 addToMap(RegionOpeningState.class, code++); > 77 addToMap(HTableDescriptor[].class, code++);
          Hide
          Ted Yu added a comment -

          This is the correct addendum.

          Show
          Ted Yu added a comment - This is the correct addendum.
          Hide
          Ted Yu added a comment -

          How about this addendum ?

          Show
          Ted Yu added a comment - How about this addendum ?
          Hide
          Benoit Sigoure added a comment -

          Actually, because entries for MultiPut and MultiResponse have been removed, it would be better to insert BitComparator in between Delete [] and HLog.Entry, and then put a placeholder next to it.

          BTW, the follow one-liner is useful to see what codes are assigned to what class, and diff the output between versions of HBase:

          awk '/code\+\+/{code+=1; print code, $0;}' src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          
          Show
          Benoit Sigoure added a comment - Actually, because entries for MultiPut and MultiResponse have been removed, it would be better to insert BitComparator in between Delete [] and HLog.Entry , and then put a placeholder next to it. BTW, the follow one-liner is useful to see what codes are assigned to what class, and diff the output between versions of HBase: awk '/code\+\+/{code+=1; print code, $0;}' src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          Hide
          Benoit Sigoure added a comment -

          Sorry I found one more problem. In HBASE-3335 another code was inserted in the middle of the list for BitComparator. Any chance that we could move this one to the end too?

          Show
          Benoit Sigoure added a comment - Sorry I found one more problem. In HBASE-3335 another code was inserted in the middle of the list for BitComparator . Any chance that we could move this one to the end too?
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #79 (See https://builds.apache.org/job/HBase-TRUNK-security/79/)
          HBASE-5204 Backward compatibility fixes for 0.92

          stack :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #79 (See https://builds.apache.org/job/HBase-TRUNK-security/79/ ) HBASE-5204 Backward compatibility fixes for 0.92 stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.92-security #77 (See https://builds.apache.org/job/HBase-0.92-security/77/)
          HBASE-5204 Backward compatibility fixes for 0.92

          stack :
          Files :

          • /hbase/branches/0.92/CHANGES.txt
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
          Show
          Hudson added a comment - Integrated in HBase-0.92-security #77 (See https://builds.apache.org/job/HBase-0.92-security/77/ ) HBASE-5204 Backward compatibility fixes for 0.92 stack : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
          Hide
          Benoit Sigoure added a comment -

          Thanks for the quick turnaround. And once again, sorry for submitting this so late in the release process.

          Show
          Benoit Sigoure added a comment - Thanks for the quick turnaround. And once again, sorry for submitting this so late in the release process.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2634 (See https://builds.apache.org/job/HBase-TRUNK/2634/)
          HBASE-5204 Backward compatibility fixes for 0.92

          stack :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2634 (See https://builds.apache.org/job/HBase-TRUNK/2634/ ) HBASE-5204 Backward compatibility fixes for 0.92 stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.92 #245 (See https://builds.apache.org/job/HBase-0.92/245/)
          HBASE-5204 Backward compatibility fixes for 0.92

          stack :
          Files :

          • /hbase/branches/0.92/CHANGES.txt
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
          Show
          Hudson added a comment - Integrated in HBase-0.92 #245 (See https://builds.apache.org/job/HBase-0.92/245/ ) HBASE-5204 Backward compatibility fixes for 0.92 stack : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
          Hide
          stack added a comment -

          Committed branch and trunk.

          Show
          stack added a comment - Committed branch and trunk.
          Hide
          stack added a comment -

          +1 on patch. TestFromClientSide works locally for me. I'm going to commit.

          Show
          stack added a comment - +1 on patch. TestFromClientSide works locally for me. I'm going to commit.
          Hide
          Ted Yu added a comment -

          @Stack, @Todd, @Gary:
          Can you take a look at the combined patches ?

          Thanks

          Show
          Ted Yu added a comment - @Stack, @Todd, @Gary: Can you take a look at the combined patches ? Thanks
          Hide
          Ted Yu added a comment -

          Running over 0.92 test suite I saw:

          Failed tests:   testClientPoolRoundRobin(org.apache.hadoop.hbase.client.TestFromClientSide): The number of versions of '[B@2ecaa79e:[B@7ac28e11 did not match 5 expected:<5> but was:<4>
            queueFailover(org.apache.hadoop.hbase.replication.TestReplication): Waited too much time for queueFailover replication
          

          But the tests passed when run individually.

          Show
          Ted Yu added a comment - Running over 0.92 test suite I saw: Failed tests: testClientPoolRoundRobin(org.apache.hadoop.hbase.client.TestFromClientSide): The number of versions of '[B@2ecaa79e:[B@7ac28e11 did not match 5 expected:<5> but was:<4> queueFailover(org.apache.hadoop.hbase.replication.TestReplication): Waited too much time for queueFailover replication But the tests passed when run individually.
          Hide
          Ted Yu added a comment -

          I ran the following tests individually and they passed:

           1550  mt -Dtest=TestMasterObserver
           1551  mt -Dtest=TestReplication
          
          Show
          Ted Yu added a comment - I ran the following tests individually and they passed: 1550 mt -Dtest=TestMasterObserver 1551 mt -Dtest=TestReplication
          Hide
          Hadoop QA added a comment -

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

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -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.replication.TestReplication
          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/766//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/766//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/766//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/12510657/5204-trunk.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -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.replication.TestReplication 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/766//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/766//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/766//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Patch for TRUNK.

          Show
          Ted Yu added a comment - Patch for TRUNK.
          Hide
          Ted Yu added a comment -

          Here is combined patch for 0.92 with suggested change in javadoc.

          Show
          Ted Yu added a comment - Here is combined patch for 0.92 with suggested change in javadoc.
          Hide
          Ted Yu added a comment -

          +1 on the patches.
          Change in javadoc for patch 003 can be done at time of commit.

          Show
          Ted Yu added a comment - +1 on the patches. Change in javadoc for patch 003 can be done at time of commit.
          Hide
          Ted Yu added a comment -

          For patch 003, I think we should discourage removing class in the static map.
          The code is a short now. We can afford keeping a lot of unused ones.

          Show
          Ted Yu added a comment - For patch 003, I think we should discourage removing class in the static map. The code is a short now. We can afford keeping a lot of unused ones.
          Hide
          Ted Yu added a comment -

          @Benoit:
          Patch 003 couldn't be applied to trunk.
          Since all 3 patches are needed, I suggest merging them into one.

          Thanks for your discovery.

          Show
          Ted Yu added a comment - @Benoit: Patch 003 couldn't be applied to trunk. Since all 3 patches are needed, I suggest merging them into one. Thanks for your discovery.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12510652/0003-Change-the-code-used-when-serializing-HTableDescript.patch
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/765//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/12510652/0003-Change-the-code-used-when-serializing-HTableDescript.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/765//console This message is automatically generated.
          Hide
          Benoit Sigoure added a comment -

          Patches to fix the issues.

          Show
          Benoit Sigoure added a comment - Patches to fix the issues.

            People

            • Assignee:
              Benoit Sigoure
              Reporter:
              Benoit Sigoure
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Due:
                Created:
                Updated:
                Resolved:

                Development