HBase
  1. HBase
  2. HBASE-5256

Use WritableUtils.readVInt() in RegionLoad.readFields()

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Currently in.readInt() is used in RegionLoad.readFields()
      More metrics would be added to RegionLoad in the future, we should utilize WritableUtils.readVInt() to reduce the amount of data exchanged between Master and region servers.

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in HBase-0.92-security #146 (See https://builds.apache.org/job/HBase-0.92-security/146/)
          HBASE-7072 HBase-5256 breaks 0.92-0.94 compatibility (Himanshu) (Revision 1404407)

          Result = FAILURE

          Show
          Hudson added a comment - Integrated in HBase-0.92-security #146 (See https://builds.apache.org/job/HBase-0.92-security/146/ ) HBASE-7072 HBase-5256 breaks 0.92-0.94 compatibility (Himanshu) (Revision 1404407) Result = FAILURE
          Hide
          Hudson added a comment -

          Integrated in HBase-0.92 #601 (See https://builds.apache.org/job/HBase-0.92/601/)
          HBASE-7072 HBase-5256 breaks 0.92-0.94 compatibility (Himanshu) (Revision 1404407)

          Result = FAILURE

          Show
          Hudson added a comment - Integrated in HBase-0.92 #601 (See https://builds.apache.org/job/HBase-0.92/601/ ) HBASE-7072 HBase-5256 breaks 0.92-0.94 compatibility (Himanshu) (Revision 1404407) Result = FAILURE
          Hide
          Lars Hofhansl added a comment -

          This was committed, marking it accordingly.

          Show
          Lars Hofhansl added a comment - This was committed, marking it accordingly.
          Hide
          Ted Yu added a comment -

          See discussion on HBASE-5795.

          Show
          Ted Yu added a comment - See discussion on HBASE-5795 .
          Hide
          Lars Hofhansl added a comment -

          Or does it now? I think that affects the public interface.

          Show
          Lars Hofhansl added a comment - Or does it now? I think that affects the public interface.
          Hide
          Lars Hofhansl added a comment -

          I see. This needs to be reverted from the 0.94 branch, otherwise it breaks compatibility with 0.92.

          Show
          Lars Hofhansl added a comment - I see. This needs to be reverted from the 0.94 branch, otherwise it breaks compatibility with 0.92.
          Hide
          Ted Yu added a comment -

          I agree this is not needed for 0.94

          Show
          Ted Yu added a comment - I agree this is not needed for 0.94
          Hide
          stack added a comment -

          This is in 0.94.0

          Show
          stack added a comment - This is in 0.94.0
          Hide
          Lars Hofhansl added a comment -

          Bumping to 0.96, since we said that 0.94 should not break compatibility with 0.92.

          Show
          Lars Hofhansl added a comment - Bumping to 0.96, since we said that 0.94 should not break compatibility with 0.92.
          Hide
          stack added a comment -

          Yes. Upgrading. Seems like it'd be easy to make this self-migrating. Would suggest we do it. Make a new issue?

          Show
          stack added a comment - Yes. Upgrading. Seems like it'd be easy to make this self-migrating. Would suggest we do it. Make a new issue?
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2649 (See https://builds.apache.org/job/HBase-TRUNK/2649/)
          HBASE-5256 Use WritableUtils.readVInt() in RegionLoad.readFields() (Mubarak)

          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HServerLoad.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2649 (See https://builds.apache.org/job/HBase-TRUNK/2649/ ) HBASE-5256 Use WritableUtils.readVInt() in RegionLoad.readFields() (Mubarak) tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HServerLoad.java
          Hide
          Ted Yu added a comment -

          @Stack:
          The patch is only for TRUNK.
          Are you concerned with upgrading from 0.90/0.92 to 0.94 ?

          Show
          Ted Yu added a comment - @Stack: The patch is only for TRUNK. Are you concerned with upgrading from 0.90/0.92 to 0.94 ?
          Hide
          stack added a comment -

          Will we have to be careful with this patch? It can't go into a minor version, right else you could have a regionserver reporting a master metrics in a format it can't interpret (or vice versa, the master will be expecting them as longs but they come over as vlongs).

          We up the version on HServerLoad but don't exploit the version change. We could have had HSL self-migrate reading using old code if it got a v1 HSL to deserialize. Then we could have this in a point version.

          Show
          stack added a comment - Will we have to be careful with this patch? It can't go into a minor version, right else you could have a regionserver reporting a master metrics in a format it can't interpret (or vice versa, the master will be expecting them as longs but they come over as vlongs). We up the version on HServerLoad but don't exploit the version change. We could have had HSL self-migrate reading using old code if it got a v1 HSL to deserialize. Then we could have this in a point version.
          Hide
          Ted Yu added a comment -

          Integrated to TRUNK.

          Thanks for the patch Mubarak.

          Show
          Ted Yu added a comment - Integrated to TRUNK. Thanks for the patch Mubarak.
          Hide
          Ted Yu added a comment -

          Will integrate the patch if there is no objection.

          Show
          Ted Yu added a comment - Will integrate the patch if there is no objection.
          Hide
          Ted Yu added a comment -
          Running org.apache.hadoop.hbase.replication.TestReplicationPeer
          Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 7.53 sec
          

          TestReplicationPeer passed locally on my MacBook.

          Patch looks good.

          Show
          Ted Yu added a comment - Running org.apache.hadoop.hbase.replication.TestReplicationPeer Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 7.53 sec TestReplicationPeer passed locally on my MacBook. Patch looks good.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12512491/HBASE-5256.trunk.v1.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 javadoc. The javadoc tool appears to have generated -140 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 161 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.replication.TestReplicationPeer
          org.apache.hadoop.hbase.io.hfile.TestHFileBlock
          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/877//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/877//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/877//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/12512491/HBASE-5256.trunk.v1.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 javadoc. The javadoc tool appears to have generated -140 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 161 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.replication.TestReplicationPeer org.apache.hadoop.hbase.io.hfile.TestHFileBlock 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/877//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/877//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/877//console This message is automatically generated.
          Hide
          Mubarak Seyed added a comment -

          The attached file is a patch.

          Show
          Mubarak Seyed added a comment - The attached file is a patch.
          Hide
          Mubarak Seyed added a comment -

          Waiting for corporate approval to contribute this patch. Thanks.

          Show
          Mubarak Seyed added a comment - Waiting for corporate approval to contribute this patch. Thanks.
          Hide
          Ted Yu added a comment -

          Since the version of RegionLoad would be bumped, I think this change should be applied to all integer/long metrics.

          Show
          Ted Yu added a comment - Since the version of RegionLoad would be bumped, I think this change should be applied to all integer/long metrics.
          Hide
          Lars Hofhansl added a comment -

          Should we recommend this only for new metrics (in order to avoid more wire incompatibilities?

          Show
          Lars Hofhansl added a comment - Should we recommend this only for new metrics (in order to avoid more wire incompatibilities?

            People

            • Assignee:
              Mubarak Seyed
              Reporter:
              Ted Yu
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development