HBase
  1. HBase
  2. HBASE-6036

Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls)

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.95.0
    • Component/s: IPC/RPC, master, migration
    • Labels:
      None

      Description

      This should be a subtask of HBASE-5445, but since that is a subtask, I can't also make this a subtask (apparently).

      Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453). These are:
      IsMasterRunning
      Shutdown
      StopMaster
      Balance
      LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch)

      1. HBASE-6036.patch
        195 kB
        Gregory Chanan
      2. HBASE-6036-v2.patch
        198 kB
        Gregory Chanan

        Issue Links

          Activity

          Hide
          stack added a comment -

          Marking closed.

          Show
          stack added a comment - Marking closed.
          Hide
          Gregory Chanan added a comment -

          Marking resolved as it has been committed to trunk. Change Status if you disagree.

          Show
          Gregory Chanan added a comment - Marking resolved as it has been committed to trunk. Change Status if you disagree.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #16 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/16/)
          HBASE-6036 Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls) (Revision 1342109)
          HBASE-6036 Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls) (Revision 1342108)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/src/main/protobuf/Master.proto

          stack :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #16 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/16/ ) HBASE-6036 Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls) (Revision 1342109) HBASE-6036 Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls) (Revision 1342108) Result = FAILURE stack : Files : /hbase/trunk/src/main/protobuf/Master.proto stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java
          Hide
          Gregory Chanan added a comment -

          Safe to mark this Resolved?

          Show
          Gregory Chanan added a comment - Safe to mark this Resolved?
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2919 (See https://builds.apache.org/job/HBase-TRUNK/2919/)
          HBASE-6036 Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls) (Revision 1342109)
          HBASE-6036 Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls) (Revision 1342108)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/src/main/protobuf/Master.proto

          stack :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2919 (See https://builds.apache.org/job/HBase-TRUNK/2919/ ) HBASE-6036 Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls) (Revision 1342109) HBASE-6036 Add Cluster-level PB-based calls to HMasterInterface (minus file-format related calls) (Revision 1342108) Result = FAILURE stack : Files : /hbase/trunk/src/main/protobuf/Master.proto stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java
          Hide
          Gregory Chanan added a comment -

          These replication tests fail even without this patch applied, so I think this is good to go.

          Show
          Gregory Chanan added a comment - These replication tests fail even without this patch applied, so I think this is good to go.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12528198/HBASE-6036-v2.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 9 new or modified tests.

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +1 javadoc. The javadoc tool did not generate any warning messages.

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

          -1 findbugs. The patch appears to introduce 32 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.TestReplication
          org.apache.hadoop.hbase.replication.TestMultiSlaveReplication
          org.apache.hadoop.hbase.replication.TestMasterReplication

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1940//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1940//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1940//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/12528198/HBASE-6036-v2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 32 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.TestReplication org.apache.hadoop.hbase.replication.TestMultiSlaveReplication org.apache.hadoop.hbase.replication.TestMasterReplication Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1940//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1940//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1940//console This message is automatically generated.
          Hide
          Gregory Chanan added a comment -

          Attached: HBASE-6036-v2.patch
          Latest version from reviewboard.

          Show
          Gregory Chanan added a comment - Attached: HBASE-6036 -v2.patch Latest version from reviewboard.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/5157/#review7985
          -----------------------------------------------------------

          src/main/protobuf/Master.proto
          <https://reviews.apache.org/r/5157/#comment17347>

          BalancerSwitchRequest, balancerSwitch the method name?

          Do whatever you think G. I'm not going to hold up the patch over this naming.

          • Michael

          On 2012-05-19 00:24:57, Gregory Chanan wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/5157/

          -----------------------------------------------------------

          (Updated 2012-05-19 00:24:57)

          Review request for hbase and Michael Stack.

          Summary

          -------

          Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453). These are:

          IsMasterRunning

          Shutdown

          StopMaster

          Balance

          LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch)

          This addresses bug HBASE-6036.

          https://issues.apache.org/jira/browse/HBASE-6036

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b

          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 07334f8

          src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java a49651b

          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 90cb53d

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403

          src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java dd18ed9

          src/main/protobuf/Master.proto PRE-CREATION

          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644

          src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5

          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java a24f937

          Diff: https://reviews.apache.org/r/5157/diff

          Testing

          -------

          Ran unit tests, all passed.

          Thanks,

          Gregory

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5157/#review7985 ----------------------------------------------------------- src/main/protobuf/Master.proto < https://reviews.apache.org/r/5157/#comment17347 > BalancerSwitchRequest, balancerSwitch the method name? Do whatever you think G. I'm not going to hold up the patch over this naming. Michael On 2012-05-19 00:24:57, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5157/ ----------------------------------------------------------- (Updated 2012-05-19 00:24:57) Review request for hbase and Michael Stack. Summary ------- Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453 ). These are: IsMasterRunning Shutdown StopMaster Balance LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch) This addresses bug HBASE-6036 . https://issues.apache.org/jira/browse/HBASE-6036 Diffs ----- src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 07334f8 src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165 src/main/java/org/apache/hadoop/hbase/master/HMaster.java a49651b src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 90cb53d src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java dd18ed9 src/main/protobuf/Master.proto PRE-CREATION src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644 src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java a24f937 Diff: https://reviews.apache.org/r/5157/diff Testing ------- Ran unit tests, all passed. Thanks, Gregory
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/5157/
          -----------------------------------------------------------

          (Updated 2012-05-19 00:24:57.479662)

          Review request for hbase and Michael Stack.

          Changes
          -------

          Update for Stack's comments and latest trunk.

          Summary
          -------

          Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453). These are:
          IsMasterRunning
          Shutdown
          StopMaster
          Balance
          LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch)

          This addresses bug HBASE-6036.
          https://issues.apache.org/jira/browse/HBASE-6036

          Diffs (updated)


          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b
          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 07334f8
          src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165
          src/main/java/org/apache/hadoop/hbase/master/HMaster.java a49651b
          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 90cb53d
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403
          src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java dd18ed9
          src/main/protobuf/Master.proto PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644
          src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5
          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java a24f937

          Diff: https://reviews.apache.org/r/5157/diff

          Testing
          -------

          Ran unit tests, all passed.

          Thanks,

          Gregory

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5157/ ----------------------------------------------------------- (Updated 2012-05-19 00:24:57.479662) Review request for hbase and Michael Stack. Changes ------- Update for Stack's comments and latest trunk. Summary ------- Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453 ). These are: IsMasterRunning Shutdown StopMaster Balance LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch) This addresses bug HBASE-6036 . https://issues.apache.org/jira/browse/HBASE-6036 Diffs (updated) src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 07334f8 src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165 src/main/java/org/apache/hadoop/hbase/master/HMaster.java a49651b src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 90cb53d src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java dd18ed9 src/main/protobuf/Master.proto PRE-CREATION src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644 src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java a24f937 Diff: https://reviews.apache.org/r/5157/diff Testing ------- Ran unit tests, all passed. Thanks, Gregory
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > Patch looks good. Minor nits below. See what you think.

          Thanks for the review.

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 704

          > <https://reviews.apache.org/r/5157/diff/1/?file=109492#file109492line704>

          >

          > FYI, convention is space after the comma – its easier to read.

          Fixed.

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1657

          > <https://reviews.apache.org/r/5157/diff/1/?file=109492#file109492line1657>

          >

          > You should write this as

          >

          > LOG.info("Checking master connection", e);

          >

          > Should it be warn?

          Changed.

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1208

          > <https://reviews.apache.org/r/5157/diff/1/?file=109494#file109494line1208>

          >

          > White space

          Fixed.

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java, line 82

          > <https://reviews.apache.org/r/5157/diff/1/?file=109493#file109493line82>

          >

          > Why we take it if unused?

          protobuf generates the function signature like that. When I implement HBASE-6039, I'm going to just take the functions as they are generated by protobufs (see RegionServerStatusProtocol for an example).

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1761

          > <https://reviews.apache.org/r/5157/diff/1/?file=109494#file109494line1761>

          >

          > No need of the controller? Would we ever need it? If not, don't pass it?

          Covered above.

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > src/main/protobuf/Master.proto, line 56

          > <https://reviews.apache.org/r/5157/diff/1/?file=109498#file109498line56>

          >

          > Should this be MasterRunningRequest rather than IsMasterRunningRequest?

          >

          > Or, is it just that you have a pattern going here where the Messages match the rpc in name?

          >

          > If the latter, thats good enough for me.

          It's the latter.

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1077

          > <https://reviews.apache.org/r/5157/diff/1/?file=109494#file109494line1077>

          >

          > Two spaces in hbase and hadoop for 'tab' . This 'return' is 4 or 6 spaces over?

          Fixed.

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java, line 236

          > <https://reviews.apache.org/r/5157/diff/1/?file=109493#file109493line236>

          >

          > Nit: You should look at the javadoc generated by this markup. You'll see that it comes out nothing like how you have it here formatted. For future.

          Fixed, thanks for pointing that out.

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 1433

          > <https://reviews.apache.org/r/5157/diff/1/?file=109491#file109491line1433>

          >

          > So, its ok changing the public-facing API because 0.96 is going to be the singularity?

          That's true, but no point in breaking clients of this class if we don't need to.

          On 2012-05-18 22:43:10, Michael Stack wrote:

          > src/main/protobuf/Master.proto, line 133

          > <https://reviews.apache.org/r/5157/diff/1/?file=109498#file109498line133>

          >

          > This method and message name is awkward. To match your IsMasterRunning, this should be IsBalancer?

          Agree that it is awkward. IsMasterRunning isn't a good match, because that is a question, whereas this controls whether the load balancer should be on or off (that is, an action). This is the old balanceSwitch(boolean).

          How about setBalancerRunning(boolean)?

          • Gregory

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/5157/#review7982
          -----------------------------------------------------------

          On 2012-05-17 20:33:52, Gregory Chanan wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/5157/

          -----------------------------------------------------------

          (Updated 2012-05-17 20:33:52)

          Review request for hbase and Michael Stack.

          Summary

          -------

          Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453). These are:

          IsMasterRunning

          Shutdown

          StopMaster

          Balance

          LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch)

          This addresses bug HBASE-6036.

          https://issues.apache.org/jira/browse/HBASE-6036

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b

          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 5cac9af

          src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 16ac781

          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 4348d20

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403

          src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 87a7a06

          src/main/protobuf/Master.proto PRE-CREATION

          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644

          src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5

          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java a24f937

          Diff: https://reviews.apache.org/r/5157/diff

          Testing

          -------

          Ran unit tests, all passed.

          Thanks,

          Gregory

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-05-18 22:43:10, Michael Stack wrote: > Patch looks good. Minor nits below. See what you think. Thanks for the review. On 2012-05-18 22:43:10, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 704 > < https://reviews.apache.org/r/5157/diff/1/?file=109492#file109492line704 > > > FYI, convention is space after the comma – its easier to read. Fixed. On 2012-05-18 22:43:10, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1657 > < https://reviews.apache.org/r/5157/diff/1/?file=109492#file109492line1657 > > > You should write this as > > LOG.info("Checking master connection", e); > > Should it be warn? Changed. On 2012-05-18 22:43:10, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1208 > < https://reviews.apache.org/r/5157/diff/1/?file=109494#file109494line1208 > > > White space Fixed. On 2012-05-18 22:43:10, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java, line 82 > < https://reviews.apache.org/r/5157/diff/1/?file=109493#file109493line82 > > > Why we take it if unused? protobuf generates the function signature like that. When I implement HBASE-6039 , I'm going to just take the functions as they are generated by protobufs (see RegionServerStatusProtocol for an example). On 2012-05-18 22:43:10, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1761 > < https://reviews.apache.org/r/5157/diff/1/?file=109494#file109494line1761 > > > No need of the controller? Would we ever need it? If not, don't pass it? Covered above. On 2012-05-18 22:43:10, Michael Stack wrote: > src/main/protobuf/Master.proto, line 56 > < https://reviews.apache.org/r/5157/diff/1/?file=109498#file109498line56 > > > Should this be MasterRunningRequest rather than IsMasterRunningRequest? > > Or, is it just that you have a pattern going here where the Messages match the rpc in name? > > If the latter, thats good enough for me. It's the latter. On 2012-05-18 22:43:10, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1077 > < https://reviews.apache.org/r/5157/diff/1/?file=109494#file109494line1077 > > > Two spaces in hbase and hadoop for 'tab' . This 'return' is 4 or 6 spaces over? Fixed. On 2012-05-18 22:43:10, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java, line 236 > < https://reviews.apache.org/r/5157/diff/1/?file=109493#file109493line236 > > > Nit: You should look at the javadoc generated by this markup. You'll see that it comes out nothing like how you have it here formatted. For future. Fixed, thanks for pointing that out. On 2012-05-18 22:43:10, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 1433 > < https://reviews.apache.org/r/5157/diff/1/?file=109491#file109491line1433 > > > So, its ok changing the public-facing API because 0.96 is going to be the singularity? That's true, but no point in breaking clients of this class if we don't need to. On 2012-05-18 22:43:10, Michael Stack wrote: > src/main/protobuf/Master.proto, line 133 > < https://reviews.apache.org/r/5157/diff/1/?file=109498#file109498line133 > > > This method and message name is awkward. To match your IsMasterRunning, this should be IsBalancer? Agree that it is awkward. IsMasterRunning isn't a good match, because that is a question, whereas this controls whether the load balancer should be on or off (that is, an action). This is the old balanceSwitch(boolean). How about setBalancerRunning(boolean)? Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5157/#review7982 ----------------------------------------------------------- On 2012-05-17 20:33:52, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5157/ ----------------------------------------------------------- (Updated 2012-05-17 20:33:52) Review request for hbase and Michael Stack. Summary ------- Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453 ). These are: IsMasterRunning Shutdown StopMaster Balance LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch) This addresses bug HBASE-6036 . https://issues.apache.org/jira/browse/HBASE-6036 Diffs ----- src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 5cac9af src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 16ac781 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 4348d20 src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 87a7a06 src/main/protobuf/Master.proto PRE-CREATION src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644 src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java a24f937 Diff: https://reviews.apache.org/r/5157/diff Testing ------- Ran unit tests, all passed. Thanks, Gregory
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12528182/HBASE-6036.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 9 new or modified tests.

          +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

          +1 javadoc. The javadoc tool did not generate any warning messages.

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

          -1 findbugs. The patch appears to introduce 32 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.TestReplication
          org.apache.hadoop.hbase.replication.TestMultiSlaveReplication
          org.apache.hadoop.hbase.replication.TestMasterReplication

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1938//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1938//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1938//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/12528182/HBASE-6036.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 32 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.TestReplication org.apache.hadoop.hbase.replication.TestMultiSlaveReplication org.apache.hadoop.hbase.replication.TestMasterReplication Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1938//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1938//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1938//console This message is automatically generated.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/5157/#review7982
          -----------------------------------------------------------

          Patch looks good. Minor nits below. See what you think.

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
          <https://reviews.apache.org/r/5157/#comment17326>

          So, its ok changing the public-facing API because 0.96 is going to be the singularity?

          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          <https://reviews.apache.org/r/5157/#comment17327>

          FYI, convention is space after the comma – its easier to read.

          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          <https://reviews.apache.org/r/5157/#comment17328>

          You should write this as

          LOG.info("Checking master connection", e);

          Should it be warn?

          src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java
          <https://reviews.apache.org/r/5157/#comment17329>

          Why we take it if unused?

          src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java
          <https://reviews.apache.org/r/5157/#comment17330>

          Nit: You should look at the javadoc generated by this markup. You'll see that it comes out nothing like how you have it here formatted. For future.

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          <https://reviews.apache.org/r/5157/#comment17331>

          Two spaces in hbase and hadoop for 'tab' . This 'return' is 4 or 6 spaces over?

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          <https://reviews.apache.org/r/5157/#comment17332>

          White space

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          <https://reviews.apache.org/r/5157/#comment17333>

          No need of the controller? Would we ever need it? If not, don't pass it?

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java
          <https://reviews.apache.org/r/5157/#comment17334>

          ditto

          src/main/protobuf/Master.proto
          <https://reviews.apache.org/r/5157/#comment17335>

          Should this be MasterRunningRequest rather than IsMasterRunningRequest?

          Or, is it just that you have a pattern going here where the Messages match the rpc in name?

          If the latter, thats good enough for me.

          src/main/protobuf/Master.proto
          <https://reviews.apache.org/r/5157/#comment17336>

          This method and message name is awkward. To match your IsMasterRunning, this should be IsBalancer?

          • Michael

          On 2012-05-17 20:33:52, Gregory Chanan wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/5157/

          -----------------------------------------------------------

          (Updated 2012-05-17 20:33:52)

          Review request for hbase and Michael Stack.

          Summary

          -------

          Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453). These are:

          IsMasterRunning

          Shutdown

          StopMaster

          Balance

          LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch)

          This addresses bug HBASE-6036.

          https://issues.apache.org/jira/browse/HBASE-6036

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b

          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 5cac9af

          src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165

          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 16ac781

          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 4348d20

          src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403

          src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 87a7a06

          src/main/protobuf/Master.proto PRE-CREATION

          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644

          src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5

          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java a24f937

          Diff: https://reviews.apache.org/r/5157/diff

          Testing

          -------

          Ran unit tests, all passed.

          Thanks,

          Gregory

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5157/#review7982 ----------------------------------------------------------- Patch looks good. Minor nits below. See what you think. src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java < https://reviews.apache.org/r/5157/#comment17326 > So, its ok changing the public-facing API because 0.96 is going to be the singularity? src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java < https://reviews.apache.org/r/5157/#comment17327 > FYI, convention is space after the comma – its easier to read. src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java < https://reviews.apache.org/r/5157/#comment17328 > You should write this as LOG.info("Checking master connection", e); Should it be warn? src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java < https://reviews.apache.org/r/5157/#comment17329 > Why we take it if unused? src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java < https://reviews.apache.org/r/5157/#comment17330 > Nit: You should look at the javadoc generated by this markup. You'll see that it comes out nothing like how you have it here formatted. For future. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/5157/#comment17331 > Two spaces in hbase and hadoop for 'tab' . This 'return' is 4 or 6 spaces over? src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/5157/#comment17332 > White space src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/5157/#comment17333 > No need of the controller? Would we ever need it? If not, don't pass it? src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/5157/#comment17334 > ditto src/main/protobuf/Master.proto < https://reviews.apache.org/r/5157/#comment17335 > Should this be MasterRunningRequest rather than IsMasterRunningRequest? Or, is it just that you have a pattern going here where the Messages match the rpc in name? If the latter, thats good enough for me. src/main/protobuf/Master.proto < https://reviews.apache.org/r/5157/#comment17336 > This method and message name is awkward. To match your IsMasterRunning, this should be IsBalancer? Michael On 2012-05-17 20:33:52, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5157/ ----------------------------------------------------------- (Updated 2012-05-17 20:33:52) Review request for hbase and Michael Stack. Summary ------- Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453 ). These are: IsMasterRunning Shutdown StopMaster Balance LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch) This addresses bug HBASE-6036 . https://issues.apache.org/jira/browse/HBASE-6036 Diffs ----- src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 5cac9af src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 16ac781 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 4348d20 src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 87a7a06 src/main/protobuf/Master.proto PRE-CREATION src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644 src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java a24f937 Diff: https://reviews.apache.org/r/5157/diff Testing ------- Ran unit tests, all passed. Thanks, Gregory
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/5157/
          -----------------------------------------------------------

          Review request for hbase and Michael Stack.

          Summary
          -------

          Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453). These are:
          IsMasterRunning
          Shutdown
          StopMaster
          Balance
          LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch)

          This addresses bug HBASE-6036.
          https://issues.apache.org/jira/browse/HBASE-6036

          Diffs


          src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b
          src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 5cac9af
          src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165
          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 16ac781
          src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 4348d20
          src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403
          src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 87a7a06
          src/main/protobuf/Master.proto PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644
          src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5
          src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java a24f937

          Diff: https://reviews.apache.org/r/5157/diff

          Testing
          -------

          Ran unit tests, all passed.

          Thanks,

          Gregory

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5157/ ----------------------------------------------------------- Review request for hbase and Michael Stack. Summary ------- Convert the cluster-level calls that do not touch the file-format related calls (see HBASE-5453 ). These are: IsMasterRunning Shutdown StopMaster Balance LoadBalancerIs (was synchronousBalanceSwitch/balanceSwitch) This addresses bug HBASE-6036 . https://issues.apache.org/jira/browse/HBASE-6036 Diffs src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 007d90b src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 5cac9af src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 80c2165 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 16ac781 src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 4348d20 src/main/java/org/apache/hadoop/hbase/protobuf/generated/MasterProtos.java 944e403 src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 87a7a06 src/main/protobuf/Master.proto PRE-CREATION src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java be52644 src/test/java/org/apache/hadoop/hbase/master/TestHMasterRPCException.java 9ff83c5 src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java a24f937 Diff: https://reviews.apache.org/r/5157/diff Testing ------- Ran unit tests, all passed. Thanks, Gregory

            People

            • Assignee:
              Gregory Chanan
              Reporter:
              Gregory Chanan
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development