Details

    • Hadoop Flags:
      Reviewed
    1. HBASE-5444-v10-trunk.patch
      357 kB
      Gregory Chanan
    2. HBASE-5444-v9-trunk.patch
      357 kB
      Gregory Chanan
    3. HBASE-5444-v6-trunk.patch
      354 kB
      Gregory Chanan

      Issue Links

        Activity

        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Review request for hbase.

        Summary
        -------

        Protobuf work for HMasterRegionInterface.

        No need to comment on the pom.xml changes: I just copied those from HBASE-5443 (https://reviews.apache.org/r/4054/).

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

        Diffs


        pom.xml 0f0aa9a
        src/main/proto/HMasterRegionProtocol.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION

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

        Testing
        -------

        mvn -DskipTests package successful and files generated successfully

        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/4149/ ----------------------------------------------------------- Review request for hbase. Summary ------- Protobuf work for HMasterRegionInterface. No need to comment on the pom.xml changes: I just copied those from HBASE-5443 ( https://reviews.apache.org/r/4054/ ). This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs pom.xml 0f0aa9a src/main/proto/HMasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4149/diff Testing ------- mvn -DskipTests package successful and files generated successfully 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/4149/#review5570
        -----------------------------------------------------------

        Please check Benoit's comments on https://reviews.apache.org/r/4054/ about shorten the names.

        I updated https://reviews.apache.org/r/4054/ with a new diff. It has ServeName and other shared protos.

        src/main/proto/HMasterRegionProtocol.proto
        <https://reviews.apache.org/r/4149/#comment12061>

        Please put generated at the end.

        • Jimmy

        On 2012-03-02 01:30:36, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-03-02 01:30:36)

        Review request for hbase.

        Summary

        -------

        Protobuf work for HMasterRegionInterface.

        No need to comment on the pom.xml changes: I just copied those from HBASE-5443 (https://reviews.apache.org/r/4054/).

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        pom.xml 0f0aa9a

        src/main/proto/HMasterRegionProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

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

        Testing

        -------

        mvn -DskipTests package successful and files generated successfully

        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/4149/#review5570 ----------------------------------------------------------- Please check Benoit's comments on https://reviews.apache.org/r/4054/ about shorten the names. I updated https://reviews.apache.org/r/4054/ with a new diff. It has ServeName and other shared protos. src/main/proto/HMasterRegionProtocol.proto < https://reviews.apache.org/r/4149/#comment12061 > Please put generated at the end. Jimmy On 2012-03-02 01:30:36, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4149/ ----------------------------------------------------------- (Updated 2012-03-02 01:30:36) Review request for hbase. Summary ------- Protobuf work for HMasterRegionInterface. No need to comment on the pom.xml changes: I just copied those from HBASE-5443 ( https://reviews.apache.org/r/4054/ ). This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- pom.xml 0f0aa9a src/main/proto/HMasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4149/diff Testing ------- mvn -DskipTests package successful and files generated successfully 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/4149/#review5574
        -----------------------------------------------------------

        src/main/proto/HMasterRegionProtocol.proto
        <https://reviews.apache.org/r/4149/#comment12072>

        Can we doc whats in here better?

        src/main/proto/HMasterRegionProtocol.proto
        <https://reviews.apache.org/r/4149/#comment12073>

        Is this one k/v only? Isn't it possible to pass more than just the one k/v? An actual Map?

        src/main/proto/HMasterRegionProtocol.proto
        <https://reviews.apache.org/r/4149/#comment12075>

        Doesn't exising interface have doc. Could we port that over?

        • Michael

        On 2012-03-02 01:30:36, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-03-02 01:30:36)

        Review request for hbase.

        Summary

        -------

        Protobuf work for HMasterRegionInterface.

        No need to comment on the pom.xml changes: I just copied those from HBASE-5443 (https://reviews.apache.org/r/4054/).

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        pom.xml 0f0aa9a

        src/main/proto/HMasterRegionProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

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

        Testing

        -------

        mvn -DskipTests package successful and files generated successfully

        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/4149/#review5574 ----------------------------------------------------------- src/main/proto/HMasterRegionProtocol.proto < https://reviews.apache.org/r/4149/#comment12072 > Can we doc whats in here better? src/main/proto/HMasterRegionProtocol.proto < https://reviews.apache.org/r/4149/#comment12073 > Is this one k/v only? Isn't it possible to pass more than just the one k/v? An actual Map? src/main/proto/HMasterRegionProtocol.proto < https://reviews.apache.org/r/4149/#comment12075 > Doesn't exising interface have doc. Could we port that over? Michael On 2012-03-02 01:30:36, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4149/ ----------------------------------------------------------- (Updated 2012-03-02 01:30:36) Review request for hbase. Summary ------- Protobuf work for HMasterRegionInterface. No need to comment on the pom.xml changes: I just copied those from HBASE-5443 ( https://reviews.apache.org/r/4054/ ). This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- pom.xml 0f0aa9a src/main/proto/HMasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4149/diff Testing ------- mvn -DskipTests package successful and files generated successfully 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/4149/#review5576
        -----------------------------------------------------------

        src/main/proto/HMasterRegionProtocol.proto
        <https://reviews.apache.org/r/4149/#comment12076>

        will do.

        src/main/proto/HMasterRegionProtocol.proto
        <https://reviews.apache.org/r/4149/#comment12077>

        You can, the name here sucks – this is really a MapEntry. If you look at RegionServerStartupResponseProto, it contains a list of these entries.

        src/main/proto/HMasterRegionProtocol.proto
        <https://reviews.apache.org/r/4149/#comment12084>

        Good idea.

        Not sure what the best form for comments is – I don't see any way to have protobufs generate javadoc comments into the generated code.

        I'll figure something out.

        • Gregory

        On 2012-03-02 01:30:36, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-03-02 01:30:36)

        Review request for hbase.

        Summary

        -------

        Protobuf work for HMasterRegionInterface.

        No need to comment on the pom.xml changes: I just copied those from HBASE-5443 (https://reviews.apache.org/r/4054/).

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        pom.xml 0f0aa9a

        src/main/proto/HMasterRegionProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

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

        Testing

        -------

        mvn -DskipTests package successful and files generated successfully

        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/4149/#review5576 ----------------------------------------------------------- src/main/proto/HMasterRegionProtocol.proto < https://reviews.apache.org/r/4149/#comment12076 > will do. src/main/proto/HMasterRegionProtocol.proto < https://reviews.apache.org/r/4149/#comment12077 > You can, the name here sucks – this is really a MapEntry. If you look at RegionServerStartupResponseProto, it contains a list of these entries. src/main/proto/HMasterRegionProtocol.proto < https://reviews.apache.org/r/4149/#comment12084 > Good idea. Not sure what the best form for comments is – I don't see any way to have protobufs generate javadoc comments into the generated code. I'll figure something out. Gregory On 2012-03-02 01:30:36, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4149/ ----------------------------------------------------------- (Updated 2012-03-02 01:30:36) Review request for hbase. Summary ------- Protobuf work for HMasterRegionInterface. No need to comment on the pom.xml changes: I just copied those from HBASE-5443 ( https://reviews.apache.org/r/4054/ ). This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- pom.xml 0f0aa9a src/main/proto/HMasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4149/diff Testing ------- mvn -DskipTests package successful and files generated successfully Thanks, Gregory
        Hide
        Ted Yu added a comment -

        I found this issue w.r.t. javadoc in protobufs generated code:
        http://code.google.com/p/protobuf/issues/detail?id=148

        Show
        Ted Yu added a comment - I found this issue w.r.t. javadoc in protobufs generated code: http://code.google.com/p/protobuf/issues/detail?id=148
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-03-02 22:18:47.273090)

        Review request for hbase.

        Summary
        -------

        Protobuf work for HMasterRegionInterface.

        No need to comment on the pom.xml changes: I just copied those from HBASE-5443 (https://reviews.apache.org/r/4054/).

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

        Diffs (updated)


        pom.xml 0f0aa9a
        src/main/proto/HMasterRegionProtocol.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION

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

        Testing
        -------

        mvn -DskipTests package successful and files generated successfully

        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/4149/ ----------------------------------------------------------- (Updated 2012-03-02 22:18:47.273090) Review request for hbase. Summary ------- Protobuf work for HMasterRegionInterface. No need to comment on the pom.xml changes: I just copied those from HBASE-5443 ( https://reviews.apache.org/r/4054/ ). This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs (updated) pom.xml 0f0aa9a src/main/proto/HMasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4149/diff Testing ------- mvn -DskipTests package successful and files generated successfully 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/4463/
        -----------------------------------------------------------

        Review request for hbase.

        Summary
        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

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

        Diffs


        pom.xml 10b13ef
        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7
        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204
        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10
        src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347
        src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68
        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830
        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f
        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2
        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7
        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838
        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7
        src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb
        src/main/proto/MasterRegionProtocol.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION
        src/main/resources/hbase-webapps/master/table.jsp 811df46
        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188
        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5
        src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a
        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c
        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing
        -------

        Ran jenkins job, all unit tests 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/4463/ ----------------------------------------------------------- Review request for hbase. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs pom.xml 10b13ef src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10 src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347 src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68 src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7 src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb src/main/proto/MasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION src/main/resources/hbase-webapps/master/table.jsp 811df46 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5 src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/4463/#review6286
        -----------------------------------------------------------

        pom.xml
        <https://reviews.apache.org/r/4463/#comment13630>

        I have some enhancement to the pom. Please check my patch on

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

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon
        <https://reviews.apache.org/r/4463/#comment13632>

        I called it HBaseProtos instead of CommonProtocol since it is not a protocol itself.

        src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java
        <https://reviews.apache.org/r/4463/#comment13633>

        Should it be Private? Should we call it MasterRegionProtocol?

        src/main/proto/MasterRegionProtocol.proto
        <https://reviews.apache.org/r/4463/#comment13634>

        This looks like a property. I have similar message for RegionProtocols. Can we put this in hbase.proto and call it Property so that we can share it?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4463/#comment13642>

        Do we have column family information?

        • Jimmy

        On 2012-03-23 04:58:51, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-03-23 04:58:51)

        Review request for hbase.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        pom.xml 10b13ef

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10

        src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347

        src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10

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

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7

        src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb

        src/main/proto/MasterRegionProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        src/main/resources/hbase-webapps/master/table.jsp 811df46

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5

        src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests 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/4463/#review6286 ----------------------------------------------------------- pom.xml < https://reviews.apache.org/r/4463/#comment13630 > I have some enhancement to the pom. Please check my patch on https://issues.apache.org/jira/browse/HBASE-5619 src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon < https://reviews.apache.org/r/4463/#comment13632 > I called it HBaseProtos instead of CommonProtocol since it is not a protocol itself. src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java < https://reviews.apache.org/r/4463/#comment13633 > Should it be Private? Should we call it MasterRegionProtocol? src/main/proto/MasterRegionProtocol.proto < https://reviews.apache.org/r/4463/#comment13634 > This looks like a property. I have similar message for RegionProtocols. Can we put this in hbase.proto and call it Property so that we can share it? src/main/proto/hbase.proto < https://reviews.apache.org/r/4463/#comment13642 > Do we have column family information? Jimmy On 2012-03-23 04:58:51, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-03-23 04:58:51) Review request for hbase. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- pom.xml 10b13ef src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10 src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347 src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68 src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7 src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb src/main/proto/MasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION src/main/resources/hbase-webapps/master/table.jsp 811df46 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5 src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests passed. Thanks, Gregory
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-23 17:01:13, Jimmy Xiang wrote:

        > src/main/proto/hbase.proto, line 33

        > <https://reviews.apache.org/r/4463/diff/1/?file=94842#file94842line33>

        >

        > Do we have column family information?

        I don't know what you mean by this question.

        I don't actually need this for HBASE-5444; I'll remove it and bring it back in HBASE-5445.

        On 2012-03-23 17:01:13, Jimmy Xiang wrote:

        > pom.xml, line 768

        > <https://reviews.apache.org/r/4463/diff/1/?file=94822#file94822line768>

        >

        > I have some enhancement to the pom. Please check my patch on

        >

        > https://issues.apache.org/jira/browse/HBASE-5619

        Will do.

        On 2012-03-23 17:01:13, Jimmy Xiang wrote:

        > src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon, line 40

        > <https://reviews.apache.org/r/4463/diff/1/?file=94823#file94823line40>

        >

        > I called it HBaseProtos instead of CommonProtocol since it is not a protocol itself.

        Sounds good, will do.

        On 2012-03-23 17:01:13, Jimmy Xiang wrote:

        > src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java, line 34

        > <https://reviews.apache.org/r/4463/diff/1/?file=94838#file94838line34>

        >

        > Should it be Private? Should we call it MasterRegionProtocol?

        I'll make it private.

        I'll leave it named Interface for now, just because it is replacing "HMasterRegionInterface." I agree we should be consistent. Perhaps someone else can chime in with an opinion on Interface vs Protocol.

        On 2012-03-23 17:01:13, Jimmy Xiang wrote:

        > src/main/proto/MasterRegionProtocol.proto, line 40

        > <https://reviews.apache.org/r/4463/diff/1/?file=94841#file94841line40>

        >

        > This looks like a property. I have similar message for RegionProtocols. Can we put this in hbase.proto and call it Property so that we can share it?

        Property is kind of a non-descriptive name. How about NameValuePair?

        • Gregory

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

        On 2012-03-23 04:58:51, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-03-23 04:58:51)

        Review request for hbase.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        pom.xml 10b13ef

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10

        src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347

        src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10

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

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7

        src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb

        src/main/proto/MasterRegionProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        src/main/resources/hbase-webapps/master/table.jsp 811df46

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5

        src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests passed.

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-23 17:01:13, Jimmy Xiang wrote: > src/main/proto/hbase.proto, line 33 > < https://reviews.apache.org/r/4463/diff/1/?file=94842#file94842line33 > > > Do we have column family information? I don't know what you mean by this question. I don't actually need this for HBASE-5444 ; I'll remove it and bring it back in HBASE-5445 . On 2012-03-23 17:01:13, Jimmy Xiang wrote: > pom.xml, line 768 > < https://reviews.apache.org/r/4463/diff/1/?file=94822#file94822line768 > > > I have some enhancement to the pom. Please check my patch on > > https://issues.apache.org/jira/browse/HBASE-5619 Will do. On 2012-03-23 17:01:13, Jimmy Xiang wrote: > src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon, line 40 > < https://reviews.apache.org/r/4463/diff/1/?file=94823#file94823line40 > > > I called it HBaseProtos instead of CommonProtocol since it is not a protocol itself. Sounds good, will do. On 2012-03-23 17:01:13, Jimmy Xiang wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java, line 34 > < https://reviews.apache.org/r/4463/diff/1/?file=94838#file94838line34 > > > Should it be Private? Should we call it MasterRegionProtocol? I'll make it private. I'll leave it named Interface for now, just because it is replacing "HMasterRegionInterface." I agree we should be consistent. Perhaps someone else can chime in with an opinion on Interface vs Protocol. On 2012-03-23 17:01:13, Jimmy Xiang wrote: > src/main/proto/MasterRegionProtocol.proto, line 40 > < https://reviews.apache.org/r/4463/diff/1/?file=94841#file94841line40 > > > This looks like a property. I have similar message for RegionProtocols. Can we put this in hbase.proto and call it Property so that we can share it? Property is kind of a non-descriptive name. How about NameValuePair? Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/#review6286 ----------------------------------------------------------- On 2012-03-23 04:58:51, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-03-23 04:58:51) Review request for hbase. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- pom.xml 10b13ef src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10 src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347 src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68 src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7 src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb src/main/proto/MasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION src/main/resources/hbase-webapps/master/table.jsp 811df46 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5 src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/4463/
        -----------------------------------------------------------

        (Updated 2012-03-23 19:55:28.089163)

        Review request for hbase.

        Changes
        -------

        Update for Jimmy's review.

        Summary
        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

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

        Diffs (updated)


        pom.xml 10b13ef
        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7
        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204
        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10
        src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347
        src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68
        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830
        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f
        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2
        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7
        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838
        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7
        src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb
        src/main/proto/MasterRegionProtocol.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION
        src/main/resources/hbase-webapps/master/table.jsp 811df46
        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188
        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5
        src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a
        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c
        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing
        -------

        Ran jenkins job, all unit tests 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/4463/ ----------------------------------------------------------- (Updated 2012-03-23 19:55:28.089163) Review request for hbase. Changes ------- Update for Jimmy's review. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs (updated) pom.xml 10b13ef src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10 src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347 src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68 src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7 src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb src/main/proto/MasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION src/main/resources/hbase-webapps/master/table.jsp 811df46 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5 src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests passed. Thanks, Gregory
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-23 17:01:13, Jimmy Xiang wrote:

        > src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java, line 34

        > <https://reviews.apache.org/r/4463/diff/1/?file=94838#file94838line34>

        >

        > Should it be Private? Should we call it MasterRegionProtocol?

        Gregory Chanan wrote:

        I'll make it private.

        I'll leave it named Interface for now, just because it is replacing "HMasterRegionInterface." I agree we should be consistent. Perhaps someone else can chime in with an opinion on Interface vs Protocol.

        IMO, Protocol would be better. Interface is way overloaded. So is protocol but less so and this seems like usage seems particularly apt. Just my twosense (This was an old comment I never published)

        • Michael

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

        On 2012-03-23 19:55:28, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-03-23 19:55:28)

        Review request for hbase.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        pom.xml 10b13ef

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10

        src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347

        src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10

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

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7

        src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb

        src/main/proto/MasterRegionProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        src/main/resources/hbase-webapps/master/table.jsp 811df46

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5

        src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests passed.

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-23 17:01:13, Jimmy Xiang wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java, line 34 > < https://reviews.apache.org/r/4463/diff/1/?file=94838#file94838line34 > > > Should it be Private? Should we call it MasterRegionProtocol? Gregory Chanan wrote: I'll make it private. I'll leave it named Interface for now, just because it is replacing "HMasterRegionInterface." I agree we should be consistent. Perhaps someone else can chime in with an opinion on Interface vs Protocol. IMO, Protocol would be better. Interface is way overloaded. So is protocol but less so and this seems like usage seems particularly apt. Just my twosense (This was an old comment I never published) Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/#review6286 ----------------------------------------------------------- On 2012-03-23 19:55:28, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-03-23 19:55:28) Review request for hbase. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- pom.xml 10b13ef src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10 src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347 src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68 src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7 src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb src/main/proto/MasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION src/main/resources/hbase-webapps/master/table.jsp 811df46 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5 src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/4463/#review6651
        -----------------------------------------------------------

        Looks great. On commit will everything be broke?

        pom.xml
        <https://reviews.apache.org/r/4463/#comment14348>

        We don't need this anymore now we are checking in generated files.

        src/main/java/org/apache/hadoop/hbase/HConstants.java
        <https://reviews.apache.org/r/4463/#comment14349>

        Should this top level class have references down into protobufs? Maybe the empty server load should be in ServerLoad or at least under o.a.h.h.protobuf

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java
        <https://reviews.apache.org/r/4463/#comment14350>

        Radical!

        Jimmy didn't remove his interface. Maybe he should have?

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

        Should it return the Response builder or the Response (don't they have same underlying 'Interface'? Return that?)

        Oh, I think I understand... must be a Builder in case we add config later?

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

        ok, yeah, here is the actual instance, not the builder

        src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java
        <https://reviews.apache.org/r/4463/#comment14354>

        You think this package right place for this? How about in master package or up above at o.a.h.h?

        Is MasterRegionInterface a good name for this Interface even? The name was made long long time ago. Didn't make sense then. Still doesn't. Should it be called RegionServerInterface or RegionServer in the master package – its the Interface RegionServers invoke on the master... whats a good name for that? RegionServerService or RegionServersInterface

        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java
        <https://reviews.apache.org/r/4463/#comment14353>

        Something called ProtobufUtil.java was added since you posted your patch. Maybe this stuff belongs in there?

        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java
        <https://reviews.apache.org/r/4463/#comment14355>

        Why not have this as static in ServerLoad?

        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java
        <https://reviews.apache.org/r/4463/#comment14356>

        ditto

        src/main/proto/MasterRegionProtocol.proto
        <https://reviews.apache.org/r/4463/#comment14357>

        MasterRegionProtocol doesn't seem like good name for this file? MasterRegionServer or RegionServerToMasterProtocol or Interface?

        Jimmy left of the 'Protocol' in his patch.

        src/main/proto/MasterRegionProtocol.proto
        <https://reviews.apache.org/r/4463/#comment14358>

        Seems like a bad name. No Region on Master?

        src/main/proto/MasterRegionProtocol.proto
        <https://reviews.apache.org/r/4463/#comment14359>

        What is this again? Should this be ServerName from hbase.proto?

        src/main/proto/MasterRegionProtocol.proto
        <https://reviews.apache.org/r/4463/#comment14360>

        ditto

        src/main/proto/MasterRegionProtocol.proto
        <https://reviews.apache.org/r/4463/#comment14361>

        RegionServerService?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4463/#comment14362>

        A file of this name has gone in already. Need to rejigger your patch?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4463/#comment14363>

        Use Jimmy's regionspec?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4463/#comment14364>

        I think jimmy made one of these already. Coordinate. I like the name of yours...

        • Michael

        On 2012-03-23 19:55:28, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-03-23 19:55:28)

        Review request for hbase.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        pom.xml 10b13ef

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10

        src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347

        src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10

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

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7

        src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb

        src/main/proto/MasterRegionProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        src/main/resources/hbase-webapps/master/table.jsp 811df46

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5

        src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests 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/4463/#review6651 ----------------------------------------------------------- Looks great. On commit will everything be broke? pom.xml < https://reviews.apache.org/r/4463/#comment14348 > We don't need this anymore now we are checking in generated files. src/main/java/org/apache/hadoop/hbase/HConstants.java < https://reviews.apache.org/r/4463/#comment14349 > Should this top level class have references down into protobufs? Maybe the empty server load should be in ServerLoad or at least under o.a.h.h.protobuf src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java < https://reviews.apache.org/r/4463/#comment14350 > Radical! Jimmy didn't remove his interface. Maybe he should have? src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4463/#comment14351 > Should it return the Response builder or the Response (don't they have same underlying 'Interface'? Return that?) Oh, I think I understand... must be a Builder in case we add config later? src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4463/#comment14352 > ok, yeah, here is the actual instance, not the builder src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java < https://reviews.apache.org/r/4463/#comment14354 > You think this package right place for this? How about in master package or up above at o.a.h.h? Is MasterRegionInterface a good name for this Interface even? The name was made long long time ago. Didn't make sense then. Still doesn't. Should it be called RegionServerInterface or RegionServer in the master package – its the Interface RegionServers invoke on the master... whats a good name for that? RegionServerService or RegionServersInterface src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java < https://reviews.apache.org/r/4463/#comment14353 > Something called ProtobufUtil.java was added since you posted your patch. Maybe this stuff belongs in there? src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java < https://reviews.apache.org/r/4463/#comment14355 > Why not have this as static in ServerLoad? src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java < https://reviews.apache.org/r/4463/#comment14356 > ditto src/main/proto/MasterRegionProtocol.proto < https://reviews.apache.org/r/4463/#comment14357 > MasterRegionProtocol doesn't seem like good name for this file? MasterRegionServer or RegionServerToMasterProtocol or Interface? Jimmy left of the 'Protocol' in his patch. src/main/proto/MasterRegionProtocol.proto < https://reviews.apache.org/r/4463/#comment14358 > Seems like a bad name. No Region on Master? src/main/proto/MasterRegionProtocol.proto < https://reviews.apache.org/r/4463/#comment14359 > What is this again? Should this be ServerName from hbase.proto? src/main/proto/MasterRegionProtocol.proto < https://reviews.apache.org/r/4463/#comment14360 > ditto src/main/proto/MasterRegionProtocol.proto < https://reviews.apache.org/r/4463/#comment14361 > RegionServerService? src/main/proto/hbase.proto < https://reviews.apache.org/r/4463/#comment14362 > A file of this name has gone in already. Need to rejigger your patch? src/main/proto/hbase.proto < https://reviews.apache.org/r/4463/#comment14363 > Use Jimmy's regionspec? src/main/proto/hbase.proto < https://reviews.apache.org/r/4463/#comment14364 > I think jimmy made one of these already. Coordinate. I like the name of yours... Michael On 2012-03-23 19:55:28, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-03-23 19:55:28) Review request for hbase. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- pom.xml 10b13ef src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10 src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347 src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68 src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7 src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb src/main/proto/MasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION src/main/resources/hbase-webapps/master/table.jsp 811df46 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5 src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests passed. Thanks, Gregory
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java, line 78

        > <https://reviews.apache.org/r/4463/diff/2/?file=95093#file95093line78>

        >

        > Radical!

        >

        > Jimmy didn't remove his interface. Maybe he should have?

        I haven't removed HRegionInterface yet. I can remove it after the admin part is also completed.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/proto/hbase.proto, line 116

        > <https://reviews.apache.org/r/4463/diff/2/?file=95105#file95105line116>

        >

        > I think jimmy made one of these already. Coordinate. I like the name of yours...

        Sure, I can rename mine.

        • Jimmy

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

        On 2012-03-23 19:55:28, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-03-23 19:55:28)

        Review request for hbase.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        pom.xml 10b13ef

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10

        src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347

        src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10

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

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7

        src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb

        src/main/proto/MasterRegionProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        src/main/resources/hbase-webapps/master/table.jsp 811df46

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5

        src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests passed.

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java, line 78 > < https://reviews.apache.org/r/4463/diff/2/?file=95093#file95093line78 > > > Radical! > > Jimmy didn't remove his interface. Maybe he should have? I haven't removed HRegionInterface yet. I can remove it after the admin part is also completed. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/proto/hbase.proto, line 116 > < https://reviews.apache.org/r/4463/diff/2/?file=95105#file95105line116 > > > I think jimmy made one of these already. Coordinate. I like the name of yours... Sure, I can rename mine. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/#review6651 ----------------------------------------------------------- On 2012-03-23 19:55:28, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-03-23 19:55:28) Review request for hbase. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- pom.xml 10b13ef src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10 src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347 src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68 src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7 src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb src/main/proto/MasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION src/main/resources/hbase-webapps/master/table.jsp 811df46 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5 src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests passed. Thanks, Gregory
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > Looks great. On commit will everything be broke?

        Should be able to commit next version; this version passed all the tests previously (things have changed since then).

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > pom.xml, line 797

        > <https://reviews.apache.org/r/4463/diff/2/?file=95085#file95085line797>

        >

        > We don't need this anymore now we are checking in generated files.

        Correct.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 33

        > <https://reviews.apache.org/r/4463/diff/2/?file=95089#file95089line33>

        >

        > Should this top level class have references down into protobufs? Maybe the empty server load should be in ServerLoad or at least under o.a.h.h.protobuf

        I'll put it under ProtobufUtil. Don't want to put it under ServerLoad because that is generated (I know we are checking in the generated files, but it would be a pain to maintain).

        On 2012-04-03 06:07:58, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4463/diff/2/?file=95096#file95096line946>

        >

        > Should it return the Response builder or the Response (don't they have same underlying 'Interface'? Return that?)

        >

        > Oh, I think I understand... must be a Builder in case we add config later?

        Yes to the second question.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java, line 30

        > <https://reviews.apache.org/r/4463/diff/2/?file=95102#file95102line30>

        >

        > Something called ProtobufUtil.java was added since you posted your patch. Maybe this stuff belongs in there?

        Correct.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java, line 56

        > <https://reviews.apache.org/r/4463/diff/2/?file=95102#file95102line56>

        >

        > Why not have this as static in ServerLoad?

        Because ServerLoad is generated; would be a pain if we need to change it in the future.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java, line 79

        > <https://reviews.apache.org/r/4463/diff/2/?file=95102#file95102line79>

        >

        > ditto

        Same as above.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/proto/hbase.proto, line 22

        > <https://reviews.apache.org/r/4463/diff/2/?file=95105#file95105line22>

        >

        > A file of this name has gone in already. Need to rejigger your patch?

        Yes.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/proto/hbase.proto, line 30

        > <https://reviews.apache.org/r/4463/diff/2/?file=95105#file95105line30>

        >

        > Use Jimmy's regionspec?

        Good call.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/proto/MasterRegionProtocol.proto, line 50

        > <https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line50>

        >

        > What is this again? Should this be ServerName from hbase.proto?

        Will do.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/proto/MasterRegionProtocol.proto, line 61

        > <https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line61>

        >

        > ditto

        Will do.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java, line 39

        > <https://reviews.apache.org/r/4463/diff/2/?file=95101#file95101line39>

        >

        > You think this package right place for this? How about in master package or up above at o.a.h.h?

        >

        > Is MasterRegionInterface a good name for this Interface even? The name was made long long time ago. Didn't make sense then. Still doesn't. Should it be called RegionServerInterface or RegionServer in the master package – its the Interface RegionServers invoke on the master... whats a good name for that? RegionServerService or RegionServersInterface

        Master package sounds like a good place for it.

        Agreed that the current name isn't good. When Jimmy split the interfaces to the region, he broke them along functionality lines, i.e. Client vs Admin. Maybe it makes sense to add something into the name that describes what the interface does functionally, rather than just the endpoints. How about RegionServerStatus

        {Service,Interface,Protocol}

        ? (I'll just copy the endings that Jimmy used). Every call in the interface basically describes the status, e.g. startup, report, reportOnError.

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/proto/MasterRegionProtocol.proto, line 1

        > <https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line1>

        >

        > MasterRegionProtocol doesn't seem like good name for this file? MasterRegionServer or RegionServerToMasterProtocol or Interface?

        >

        > Jimmy left of the 'Protocol' in his patch.

        How about RegionServerStatus here too? (with proper ending)

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/proto/MasterRegionProtocol.proto, line 22

        > <https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line22>

        >

        > Seems like a bad name. No Region on Master?

        RegionServerStatus here too (with proper ending)?

        On 2012-04-03 06:07:58, Michael Stack wrote:

        > src/main/proto/MasterRegionProtocol.proto, line 70

        > <https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line70>

        >

        > RegionServerService?

        RegionServerStatus here too (with proper ending)?

        • Gregory

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

        On 2012-03-23 19:55:28, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-03-23 19:55:28)

        Review request for hbase.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        pom.xml 10b13ef

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10

        src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347

        src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10

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

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7

        src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb

        src/main/proto/MasterRegionProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        src/main/resources/hbase-webapps/master/table.jsp 811df46

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5

        src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests passed.

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-03 06:07:58, Michael Stack wrote: > Looks great. On commit will everything be broke? Should be able to commit next version; this version passed all the tests previously (things have changed since then). On 2012-04-03 06:07:58, Michael Stack wrote: > pom.xml, line 797 > < https://reviews.apache.org/r/4463/diff/2/?file=95085#file95085line797 > > > We don't need this anymore now we are checking in generated files. Correct. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 33 > < https://reviews.apache.org/r/4463/diff/2/?file=95089#file95089line33 > > > Should this top level class have references down into protobufs? Maybe the empty server load should be in ServerLoad or at least under o.a.h.h.protobuf I'll put it under ProtobufUtil. Don't want to put it under ServerLoad because that is generated (I know we are checking in the generated files, but it would be a pain to maintain). On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 946 > < https://reviews.apache.org/r/4463/diff/2/?file=95096#file95096line946 > > > Should it return the Response builder or the Response (don't they have same underlying 'Interface'? Return that?) > > Oh, I think I understand... must be a Builder in case we add config later? Yes to the second question. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java, line 30 > < https://reviews.apache.org/r/4463/diff/2/?file=95102#file95102line30 > > > Something called ProtobufUtil.java was added since you posted your patch. Maybe this stuff belongs in there? Correct. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java, line 56 > < https://reviews.apache.org/r/4463/diff/2/?file=95102#file95102line56 > > > Why not have this as static in ServerLoad? Because ServerLoad is generated; would be a pain if we need to change it in the future. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java, line 79 > < https://reviews.apache.org/r/4463/diff/2/?file=95102#file95102line79 > > > ditto Same as above. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/proto/hbase.proto, line 22 > < https://reviews.apache.org/r/4463/diff/2/?file=95105#file95105line22 > > > A file of this name has gone in already. Need to rejigger your patch? Yes. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/proto/hbase.proto, line 30 > < https://reviews.apache.org/r/4463/diff/2/?file=95105#file95105line30 > > > Use Jimmy's regionspec? Good call. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/proto/MasterRegionProtocol.proto, line 50 > < https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line50 > > > What is this again? Should this be ServerName from hbase.proto? Will do. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/proto/MasterRegionProtocol.proto, line 61 > < https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line61 > > > ditto Will do. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java, line 39 > < https://reviews.apache.org/r/4463/diff/2/?file=95101#file95101line39 > > > You think this package right place for this? How about in master package or up above at o.a.h.h? > > Is MasterRegionInterface a good name for this Interface even? The name was made long long time ago. Didn't make sense then. Still doesn't. Should it be called RegionServerInterface or RegionServer in the master package – its the Interface RegionServers invoke on the master... whats a good name for that? RegionServerService or RegionServersInterface Master package sounds like a good place for it. Agreed that the current name isn't good. When Jimmy split the interfaces to the region, he broke them along functionality lines, i.e. Client vs Admin. Maybe it makes sense to add something into the name that describes what the interface does functionally, rather than just the endpoints. How about RegionServerStatus {Service,Interface,Protocol} ? (I'll just copy the endings that Jimmy used). Every call in the interface basically describes the status, e.g. startup, report, reportOnError. On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/proto/MasterRegionProtocol.proto, line 1 > < https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line1 > > > MasterRegionProtocol doesn't seem like good name for this file? MasterRegionServer or RegionServerToMasterProtocol or Interface? > > Jimmy left of the 'Protocol' in his patch. How about RegionServerStatus here too? (with proper ending) On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/proto/MasterRegionProtocol.proto, line 22 > < https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line22 > > > Seems like a bad name. No Region on Master? RegionServerStatus here too (with proper ending)? On 2012-04-03 06:07:58, Michael Stack wrote: > src/main/proto/MasterRegionProtocol.proto, line 70 > < https://reviews.apache.org/r/4463/diff/2/?file=95104#file95104line70 > > > RegionServerService? RegionServerStatus here too (with proper ending)? Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/#review6651 ----------------------------------------------------------- On 2012-03-23 19:55:28, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-03-23 19:55:28) Review request for hbase. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- pom.xml 10b13ef src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 9df4c10 src/main/java/org/apache/hadoop/hbase/HConstants.java 8888347 src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 2916d68 src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java d47ef10 src/main/java/org/apache/hadoop/hbase/master/HMaster.java cd1755f src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java cbd55f7 src/main/java/org/apache/hadoop/hbase/protobuf/MasterRegionInterface.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/protobuf/PBHelper.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java e0af8fb src/main/proto/MasterRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION src/main/resources/hbase-webapps/master/table.jsp 811df46 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 6af9188 src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java 368a0e5 src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 841649a src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java 379f70c src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/4463/
        -----------------------------------------------------------

        (Updated 2012-04-26 23:27:07.073810)

        Review request for hbase.

        Changes
        -------

        Updated for Stack's comments.

        Passes all unit tests.

        Summary
        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

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

        Diffs (updated)


        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7
        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091
        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b
        src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb
        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830
        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2
        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7
        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838
        src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1
        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d
        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6
        src/main/protobuf/RegionServerStatus.proto PRE-CREATION
        src/main/protobuf/hbase.proto 12e6053
        src/main/resources/hbase-webapps/master/table.jsp 3ef1190
        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb
        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90
        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing
        -------

        Ran jenkins job, all unit tests 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/4463/ ----------------------------------------------------------- (Updated 2012-04-26 23:27:07.073810) Review request for hbase. Changes ------- Updated for Stack's comments. Passes all unit tests. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs (updated) src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/4463/
        -----------------------------------------------------------

        (Updated 2012-05-01 01:29:14.986766)

        Review request for hbase and Michael Stack.

        Summary
        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

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

        Diffs


        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7
        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091
        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b
        src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb
        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830
        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2
        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7
        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838
        src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1
        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d
        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6
        src/main/protobuf/RegionServerStatus.proto PRE-CREATION
        src/main/protobuf/hbase.proto 12e6053
        src/main/resources/hbase-webapps/master/table.jsp 3ef1190
        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb
        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90
        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing
        -------

        Ran jenkins job, all unit tests 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/4463/ ----------------------------------------------------------- (Updated 2012-05-01 01:29:14.986766) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/12525149/HBASE-5444-v6-trunk.patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 18 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 3 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.client.TestShell
        org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1702//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1702//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1702//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/12525149/HBASE-5444-v6-trunk.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 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 3 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.client.TestShell org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1702//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1702//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1702//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/4463/#review7411
        -----------------------------------------------------------

        src/main/resources/hbase-webapps/master/table.jsp
        <https://reviews.apache.org/r/4463/#comment16343>

        Can this computation be moved somewhere so that it can be reused ?

        • Ted

        On 2012-05-01 01:29:14, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-05-01 01:29:14)

        Review request for hbase and Michael Stack.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b

        src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6

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

        src/main/protobuf/hbase.proto 12e6053

        src/main/resources/hbase-webapps/master/table.jsp 3ef1190

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests 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/4463/#review7411 ----------------------------------------------------------- src/main/resources/hbase-webapps/master/table.jsp < https://reviews.apache.org/r/4463/#comment16343 > Can this computation be moved somewhere so that it can be reused ? Ted On 2012-05-01 01:29:14, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-05-01 01:29:14) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/4463/
        -----------------------------------------------------------

        (Updated 2012-05-01 19:53:51.307399)

        Review request for hbase and Michael Stack.

        Changes
        -------

        Update against newest trunk and followed Ted's suggestion about breaking out totalRequestsCount computation into own function.

        Summary
        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

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

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb
        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830
        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2
        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7
        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838
        src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1
        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d
        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b
        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7
        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091
        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6
        src/main/protobuf/RegionServerStatus.proto PRE-CREATION
        src/main/protobuf/hbase.proto 12e6053
        src/main/resources/hbase-webapps/master/table.jsp 3ef1190
        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb
        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90
        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing
        -------

        Ran jenkins job, all unit tests 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/4463/ ----------------------------------------------------------- (Updated 2012-05-01 19:53:51.307399) Review request for hbase and Michael Stack. Changes ------- Update against newest trunk and followed Ted's suggestion about breaking out totalRequestsCount computation into own function. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs (updated) src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/4463/#review7441
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
        <https://reviews.apache.org/r/4463/#comment16369>

        Insert a space between if and (.

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
        <https://reviews.apache.org/r/4463/#comment16372>

        You can return list.toArray() directly, similar to line 1179.

        • Ted

        On 2012-05-01 19:53:51, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-05-01 19:53:51)

        Review request for hbase and Michael Stack.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6

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

        src/main/protobuf/hbase.proto 12e6053

        src/main/resources/hbase-webapps/master/table.jsp 3ef1190

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests 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/4463/#review7441 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java < https://reviews.apache.org/r/4463/#comment16369 > Insert a space between if and (. src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java < https://reviews.apache.org/r/4463/#comment16372 > You can return list.toArray() directly, similar to line 1179. Ted On 2012-05-01 19:53:51, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-05-01 19:53:51) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/4463/#review7442
        -----------------------------------------------------------

        A few comments below the thrust of which are about encapsulating pb if possible rather than have it spread around in classes. See what you think. It would not be hard to get me commit this as is.

        src/main/java/org/apache/hadoop/hbase/HConstants.java
        <https://reviews.apache.org/r/4463/#comment16370>

        Were you going to move this down to where its used G? Does it need to be up here?

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java
        <https://reviews.apache.org/r/4463/#comment16371>

        Hurray!

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java
        <https://reviews.apache.org/r/4463/#comment16373>

        All of these classes are importing generated pb classes. Would it be better to have a high-level ServerLoad class that hid inside it the pb stuff instead? Less pb generated class pollution.

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
        <https://reviews.apache.org/r/4463/#comment16374>

        Yeah, its kinda ugly having this package reach over into pb package.

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        <https://reviews.apache.org/r/4463/#comment16375>

        We should not be reaching over into the master package. Put this protocol class at the top level since shared by master and regionserver?

        • Michael

        On 2012-05-01 19:53:51, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-05-01 19:53:51)

        Review request for hbase and Michael Stack.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6

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

        src/main/protobuf/hbase.proto 12e6053

        src/main/resources/hbase-webapps/master/table.jsp 3ef1190

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests 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/4463/#review7442 ----------------------------------------------------------- A few comments below the thrust of which are about encapsulating pb if possible rather than have it spread around in classes. See what you think. It would not be hard to get me commit this as is. src/main/java/org/apache/hadoop/hbase/HConstants.java < https://reviews.apache.org/r/4463/#comment16370 > Were you going to move this down to where its used G? Does it need to be up here? src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java < https://reviews.apache.org/r/4463/#comment16371 > Hurray! src/main/java/org/apache/hadoop/hbase/master/MXBean.java < https://reviews.apache.org/r/4463/#comment16373 > All of these classes are importing generated pb classes. Would it be better to have a high-level ServerLoad class that hid inside it the pb stuff instead? Less pb generated class pollution. src/main/java/org/apache/hadoop/hbase/master/ServerManager.java < https://reviews.apache.org/r/4463/#comment16374 > Yeah, its kinda ugly having this package reach over into pb package. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java < https://reviews.apache.org/r/4463/#comment16375 > We should not be reaching over into the master package. Put this protocol class at the top level since shared by master and regionserver? Michael On 2012-05-01 19:53:51, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-05-01 19:53:51) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests passed. Thanks, Gregory
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-01 20:20:09, Michael Stack wrote:

        > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 679

        > <https://reviews.apache.org/r/4463/diff/4/?file=105802#file105802line679>

        >

        > Were you going to move this down to where its used G? Does it need to be up here?

        I can just get rid of this and use null everywhere. That's what it looks like Jimmy did.

        On 2012-05-01 20:20:09, Michael Stack wrote:

        > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 173

        > <https://reviews.apache.org/r/4463/diff/4/?file=105817#file105817line173>

        >

        > We should not be reaching over into the master package. Put this protocol class at the top level since shared by master and regionserver?

        Will do.

        On 2012-05-01 20:20:09, Michael Stack wrote:

        > src/main/java/org/apache/hadoop/hbase/master/MXBean.java, line 23

        > <https://reviews.apache.org/r/4463/diff/4/?file=105809#file105809line23>

        >

        > All of these classes are importing generated pb classes. Would it be better to have a high-level ServerLoad class that hid inside it the pb stuff instead? Less pb generated class pollution.

        I think that's a good idea. I was considering doing that, but wasn't sure if I should do it for all the generated pb types. Since ServerLoad is sprinkled everywhere, it seems like that is at least a good place to start.

        • Gregory

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

        On 2012-05-01 19:53:51, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-05-01 19:53:51)

        Review request for hbase and Michael Stack.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6

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

        src/main/protobuf/hbase.proto 12e6053

        src/main/resources/hbase-webapps/master/table.jsp 3ef1190

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests passed.

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-01 20:20:09, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 679 > < https://reviews.apache.org/r/4463/diff/4/?file=105802#file105802line679 > > > Were you going to move this down to where its used G? Does it need to be up here? I can just get rid of this and use null everywhere. That's what it looks like Jimmy did. On 2012-05-01 20:20:09, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 173 > < https://reviews.apache.org/r/4463/diff/4/?file=105817#file105817line173 > > > We should not be reaching over into the master package. Put this protocol class at the top level since shared by master and regionserver? Will do. On 2012-05-01 20:20:09, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/MXBean.java, line 23 > < https://reviews.apache.org/r/4463/diff/4/?file=105809#file105809line23 > > > All of these classes are importing generated pb classes. Would it be better to have a high-level ServerLoad class that hid inside it the pb stuff instead? Less pb generated class pollution. I think that's a good idea. I was considering doing that, but wasn't sure if I should do it for all the generated pb types. Since ServerLoad is sprinkled everywhere, it seems like that is at least a good place to start. Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/#review7442 ----------------------------------------------------------- On 2012-05-01 19:53:51, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-05-01 19:53:51) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests passed. Thanks, Gregory
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-01 20:16:06, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 1128

        > <https://reviews.apache.org/r/4463/diff/4/?file=105814#file105814line1128>

        >

        > Insert a space between if and (.

        Will do.

        On 2012-05-01 20:16:06, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 1145

        > <https://reviews.apache.org/r/4463/diff/4/?file=105814#file105814line1145>

        >

        > You can return list.toArray() directly, similar to line 1179.

        I still need to convert from Coprocessor to String (via getName) if I call toArray(). Am I missing something?

        • Gregory

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

        On 2012-05-01 19:53:51, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-05-01 19:53:51)

        Review request for hbase and Michael Stack.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6

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

        src/main/protobuf/hbase.proto 12e6053

        src/main/resources/hbase-webapps/master/table.jsp 3ef1190

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests passed.

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-01 20:16:06, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 1128 > < https://reviews.apache.org/r/4463/diff/4/?file=105814#file105814line1128 > > > Insert a space between if and (. Will do. On 2012-05-01 20:16:06, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 1145 > < https://reviews.apache.org/r/4463/diff/4/?file=105814#file105814line1145 > > > You can return list.toArray() directly, similar to line 1179. I still need to convert from Coprocessor to String (via getName) if I call toArray(). Am I missing something? Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/#review7441 ----------------------------------------------------------- On 2012-05-01 19:53:51, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-05-01 19:53:51) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0 src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/4463/
        -----------------------------------------------------------

        (Updated 2012-05-02 23:19:20.036448)

        Review request for hbase and Michael Stack.

        Changes
        -------

        Make changes based on Ted's and Stack's comments.

        • Fixed spacing
        • Moved RegionServerStatusProtocol from master to ipc (not o.a.h.h – hopefully that's okay)
        • Created a ServerLoad object that encapsulated the PB ServerLoad object.

        Summary
        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

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

        Diffs (updated)


        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7
        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091
        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b
        src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb
        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830
        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b
        src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2
        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7
        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838
        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1
        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d
        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6
        src/main/protobuf/RegionServerStatus.proto PRE-CREATION
        src/main/protobuf/hbase.proto 12e6053
        src/main/resources/hbase-webapps/master/table.jsp 3ef1190
        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb
        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90
        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing
        -------

        Ran jenkins job, all unit tests 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/4463/ ----------------------------------------------------------- (Updated 2012-05-02 23:19:20.036448) Review request for hbase and Michael Stack. Changes ------- Make changes based on Ted's and Stack's comments. Fixed spacing Moved RegionServerStatusProtocol from master to ipc (not o.a.h.h – hopefully that's okay) Created a ServerLoad object that encapsulated the PB ServerLoad object. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs (updated) src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/4463/#review7492
        -----------------------------------------------------------

        I'm +1 on this patch. I think this much cleaner than previous versions. I wanted more, of course, where users of the protocol would somehow be untouched or polluted by pbs but I realize that is asking for to much. Good stuff Gregory.

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java
        <https://reviews.apache.org/r/4463/#comment16588>

        We need this import? Its for cp. Thats ok I'd say.... One day we can hide that too..

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

        We can move the ipc protocol stuff to top level later... I was thinking that these classes shared by master and regionservers could be at o.a.h.h... but can do that later if it makes sense. Lets get this pb stuff in first.

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

        We need this?

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

        Yeah, I suppose you can't hide these from the class that is implementing the protocol...

        • Michael

        On 2012-05-02 23:19:20, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-05-02 23:19:20)

        Review request for hbase and Michael Stack.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b

        src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b

        src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6

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

        src/main/protobuf/hbase.proto 12e6053

        src/main/resources/hbase-webapps/master/table.jsp 3ef1190

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests 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/4463/#review7492 ----------------------------------------------------------- I'm +1 on this patch. I think this much cleaner than previous versions. I wanted more, of course, where users of the protocol would somehow be untouched or polluted by pbs but I realize that is asking for to much. Good stuff Gregory. src/main/java/org/apache/hadoop/hbase/ClusterStatus.java < https://reviews.apache.org/r/4463/#comment16588 > We need this import? Its for cp. Thats ok I'd say.... One day we can hide that too.. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4463/#comment16590 > We can move the ipc protocol stuff to top level later... I was thinking that these classes shared by master and regionservers could be at o.a.h.h... but can do that later if it makes sense. Lets get this pb stuff in first. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4463/#comment16589 > We need this? src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4463/#comment16591 > Yeah, I suppose you can't hide these from the class that is implementing the protocol... Michael On 2012-05-02 23:19:20, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-05-02 23:19:20) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests passed. Thanks, Gregory
        Hide
        stack added a comment -

        Is v6 what is up in rb? And you've submitted it to hadoopqa? Thanks Gregory.

        Show
        stack added a comment - Is v6 what is up in rb? And you've submitted it to hadoopqa? Thanks Gregory.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-02 23:43:07, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4463/diff/5/?file=106063#file106063line79>

        >

        > We can move the ipc protocol stuff to top level later... I was thinking that these classes shared by master and regionservers could be at o.a.h.h... but can do that later if it makes sense. Lets get this pb stuff in first.

        >

        >

        Sounds good.

        On 2012-05-02 23:43:07, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4463/diff/5/?file=106063#file106063line80>

        >

        > We need this?

        I use it to convert the PB serverName that is passed into HMaster.regionServerReport into a ServerName that the ServerManager understands. Instead, we could have a ServerName static function that takes a PB ServerName and returns a ServerName. We already have a bunch of these parse* functions already, e.g.

        public static ServerName parseVersionedServerName(final byte [] versionedBytes)

        On 2012-05-02 23:43:07, Michael Stack wrote:

        > src/main/java/org/apache/hadoop/hbase/ClusterStatus.java, line 36

        > <https://reviews.apache.org/r/4463/diff/5/?file=106055#file106055line36>

        >

        > We need this import? Its for cp. Thats ok I'd say.... One day we can hide that too..

        I think we can get rid of this once I pb-ify the HMasterInterface. I'll put it on my list to check out. There's probably a few such cases.

        • Gregory

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

        On 2012-05-02 23:19:20, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-05-02 23:19:20)

        Review request for hbase and Michael Stack.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b

        src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b

        src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6

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

        src/main/protobuf/hbase.proto 12e6053

        src/main/resources/hbase-webapps/master/table.jsp 3ef1190

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests passed.

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-02 23:43:07, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 79 > < https://reviews.apache.org/r/4463/diff/5/?file=106063#file106063line79 > > > We can move the ipc protocol stuff to top level later... I was thinking that these classes shared by master and regionservers could be at o.a.h.h... but can do that later if it makes sense. Lets get this pb stuff in first. > > Sounds good. On 2012-05-02 23:43:07, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 80 > < https://reviews.apache.org/r/4463/diff/5/?file=106063#file106063line80 > > > We need this? I use it to convert the PB serverName that is passed into HMaster.regionServerReport into a ServerName that the ServerManager understands. Instead, we could have a ServerName static function that takes a PB ServerName and returns a ServerName. We already have a bunch of these parse* functions already, e.g. public static ServerName parseVersionedServerName(final byte [] versionedBytes) On 2012-05-02 23:43:07, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/ClusterStatus.java, line 36 > < https://reviews.apache.org/r/4463/diff/5/?file=106055#file106055line36 > > > We need this import? Its for cp. Thats ok I'd say.... One day we can hide that too.. I think we can get rid of this once I pb-ify the HMasterInterface. I'll put it on my list to check out. There's probably a few such cases. Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/#review7492 ----------------------------------------------------------- On 2012-05-02 23:19:20, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-05-02 23:19:20) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests 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/12525367/HBASE-5444-v9-trunk.patch
        against trunk revision .

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

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

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

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1733//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/12525367/HBASE-5444-v9-trunk.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1733//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-02 23:43:07, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4463/diff/5/?file=106063#file106063line80>

        >

        > We need this?

        Gregory Chanan wrote:

        I use it to convert the PB serverName that is passed into HMaster.regionServerReport into a ServerName that the ServerManager understands. Instead, we could have a ServerName static function that takes a PB ServerName and returns a ServerName. We already have a bunch of these parse* functions already, e.g.

        public static ServerName parseVersionedServerName(final byte [] versionedBytes)

        Recently in trunk, we added a ServerName.parseFrom that should be able to make sense of any set of bytes parsed it whether pbs or old style versioned bytes. Could use this.

        • Michael

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

        On 2012-05-02 23:19:20, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-05-02 23:19:20)

        Review request for hbase and Michael Stack.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b

        src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b

        src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6

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

        src/main/protobuf/hbase.proto 12e6053

        src/main/resources/hbase-webapps/master/table.jsp 3ef1190

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests passed.

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-02 23:43:07, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 80 > < https://reviews.apache.org/r/4463/diff/5/?file=106063#file106063line80 > > > We need this? Gregory Chanan wrote: I use it to convert the PB serverName that is passed into HMaster.regionServerReport into a ServerName that the ServerManager understands. Instead, we could have a ServerName static function that takes a PB ServerName and returns a ServerName. We already have a bunch of these parse* functions already, e.g. public static ServerName parseVersionedServerName(final byte [] versionedBytes) Recently in trunk, we added a ServerName.parseFrom that should be able to make sense of any set of bytes parsed it whether pbs or old style versioned bytes. Could use this. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/#review7492 ----------------------------------------------------------- On 2012-05-02 23:19:20, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-05-02 23:19:20) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests passed. Thanks, Gregory
        Hide
        stack added a comment -

        Sorry Gregory, I meant to ask, would you like me commit this and then in separate issues work on the outstanding stuff or do you want to update a new patch?

        Show
        stack added a comment - Sorry Gregory, I meant to ask, would you like me commit this and then in separate issues work on the outstanding stuff or do you want to update a new patch?
        Hide
        Gregory Chanan added a comment -

        Updated version against newest trunk.

        I would like you to commit this and then I'll work on separate issues on the outstanding stuff.

        Show
        Gregory Chanan added a comment - Updated version against newest trunk. I would like you to commit this and then I'll work on separate issues on the outstanding stuff.
        Hide
        stack added a comment -

        @Gregory np Please file the other issues. Waiting on hadoopqa before committing. Good stuff.

        Show
        stack added a comment - @Gregory np Please file the other issues. Waiting on hadoopqa before committing. Good stuff.
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 18 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 does not introduce any 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 passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1740//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1740//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1740//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/12525392/HBASE-5444-v10-trunk.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 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 does not introduce any 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1740//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1740//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1740//console This message is automatically generated.
        Hide
        stack added a comment -

        Committed to trunk. Thanks for the patch Gregory.

        Show
        stack added a comment - Committed to trunk. Thanks for the patch Gregory.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2842 (See https://builds.apache.org/job/HBase-TRUNK/2842/)
        HBASE-5444 Add PB-based calls to HMasterRegionInterface (Revision 1333319)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon
        • /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ClusterStatus.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ServerLoad.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MXBean.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/src/main/protobuf/RegionServerStatus.proto
        • /hbase/trunk/src/main/protobuf/hbase.proto
        • /hbase/trunk/src/main/resources/hbase-webapps/master/table.jsp
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2842 (See https://builds.apache.org/job/HBase-TRUNK/2842/ ) HBASE-5444 Add PB-based calls to HMasterRegionInterface (Revision 1333319) Result = FAILURE stack : Files : /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ClusterStatus.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ServerLoad.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MXBean.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/protobuf/RegionServerStatus.proto /hbase/trunk/src/main/protobuf/hbase.proto /hbase/trunk/src/main/resources/hbase-webapps/master/table.jsp /hbase/trunk/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-02 23:43:07, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4463/diff/5/?file=106063#file106063line80>

        >

        > We need this?

        Gregory Chanan wrote:

        I use it to convert the PB serverName that is passed into HMaster.regionServerReport into a ServerName that the ServerManager understands. Instead, we could have a ServerName static function that takes a PB ServerName and returns a ServerName. We already have a bunch of these parse* functions already, e.g.

        public static ServerName parseVersionedServerName(final byte [] versionedBytes)

        Michael Stack wrote:

        Recently in trunk, we added a ServerName.parseFrom that should be able to make sense of any set of bytes parsed it whether pbs or old style versioned bytes. Could use this.

        I don't think this applies. From reading ServerName.parseFrom it looks like it requires the PBMagicPrefix, which this case doesn't have nor need. Am I missing something?

        • Gregory

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

        On 2012-05-02 23:19:20, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-05-02 23:19:20)

        Review request for hbase and Michael Stack.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b

        src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b

        src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6

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

        src/main/protobuf/hbase.proto 12e6053

        src/main/resources/hbase-webapps/master/table.jsp 3ef1190

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests passed.

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-02 23:43:07, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 80 > < https://reviews.apache.org/r/4463/diff/5/?file=106063#file106063line80 > > > We need this? Gregory Chanan wrote: I use it to convert the PB serverName that is passed into HMaster.regionServerReport into a ServerName that the ServerManager understands. Instead, we could have a ServerName static function that takes a PB ServerName and returns a ServerName. We already have a bunch of these parse* functions already, e.g. public static ServerName parseVersionedServerName(final byte [] versionedBytes) Michael Stack wrote: Recently in trunk, we added a ServerName.parseFrom that should be able to make sense of any set of bytes parsed it whether pbs or old style versioned bytes. Could use this. I don't think this applies. From reading ServerName.parseFrom it looks like it requires the PBMagicPrefix, which this case doesn't have nor need. Am I missing something? Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/#review7492 ----------------------------------------------------------- On 2012-05-02 23:19:20, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-05-02 23:19:20) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests passed. Thanks, Gregory
        Hide
        Gregory Chanan added a comment -

        @Stack:

        I filed HBASE-5932 and HBASE-5933. If you think there are things I missed either let me know or file a JIRA and assign it to me.

        Show
        Gregory Chanan added a comment - @Stack: I filed HBASE-5932 and HBASE-5933 . If you think there are things I missed either let me know or file a JIRA and assign it to me.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-02 23:43:07, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4463/diff/5/?file=106063#file106063line80>

        >

        > We need this?

        Gregory Chanan wrote:

        I use it to convert the PB serverName that is passed into HMaster.regionServerReport into a ServerName that the ServerManager understands. Instead, we could have a ServerName static function that takes a PB ServerName and returns a ServerName. We already have a bunch of these parse* functions already, e.g.

        public static ServerName parseVersionedServerName(final byte [] versionedBytes)

        Michael Stack wrote:

        Recently in trunk, we added a ServerName.parseFrom that should be able to make sense of any set of bytes parsed it whether pbs or old style versioned bytes. Could use this.

        Gregory Chanan wrote:

        I don't think this applies. From reading ServerName.parseFrom it looks like it requires the PBMagicPrefix, which this case doesn't have nor need. Am I missing something?

        If no pb magic, it then goes on to try and parse the bytes otherwise. See other side of the pb check starting at line #332.

        • Michael

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

        On 2012-05-02 23:19:20, Gregory Chanan wrote:

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

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

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

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

        (Updated 2012-05-02 23:19:20)

        Review request for hbase and Michael Stack.

        Summary

        -------

        Adds PB-based calls replacing HMasterRegionInterface.

        There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445.

        This addresses bug HBASE-5444.

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

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7

        src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091

        src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b

        src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760

        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb

        src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830

        src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b

        src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023

        src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2

        src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7

        src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838

        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1

        src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d

        src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6

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

        src/main/protobuf/hbase.proto 12e6053

        src/main/resources/hbase-webapps/master/table.jsp 3ef1190

        src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb

        src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8

        src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90

        src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba

        src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251

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

        Testing

        -------

        Ran jenkins job, all unit tests passed.

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-02 23:43:07, Michael Stack wrote: > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 80 > < https://reviews.apache.org/r/4463/diff/5/?file=106063#file106063line80 > > > We need this? Gregory Chanan wrote: I use it to convert the PB serverName that is passed into HMaster.regionServerReport into a ServerName that the ServerManager understands. Instead, we could have a ServerName static function that takes a PB ServerName and returns a ServerName. We already have a bunch of these parse* functions already, e.g. public static ServerName parseVersionedServerName(final byte [] versionedBytes) Michael Stack wrote: Recently in trunk, we added a ServerName.parseFrom that should be able to make sense of any set of bytes parsed it whether pbs or old style versioned bytes. Could use this. Gregory Chanan wrote: I don't think this applies. From reading ServerName.parseFrom it looks like it requires the PBMagicPrefix, which this case doesn't have nor need. Am I missing something? If no pb magic, it then goes on to try and parse the bytes otherwise. See other side of the pb check starting at line #332. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/#review7492 ----------------------------------------------------------- On 2012-05-02 23:19:20, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4463/ ----------------------------------------------------------- (Updated 2012-05-02 23:19:20) Review request for hbase and Michael Stack. Summary ------- Adds PB-based calls replacing HMasterRegionInterface. There are some temporary hacks, e.g. converting PB-based ServerLoad to existing HServerLoad so I didn't need to convert ClusterStatus (which brings in a lot of other changes). That will be cleaned up in HBASE-5445 . This addresses bug HBASE-5444 . https://issues.apache.org/jira/browse/HBASE-5444 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon 69434f7 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon 3c7c091 src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 5d7f07b src/main/java/org/apache/hadoop/hbase/ServerLoad.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java 0db2760 src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 973c7cb src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java fd97830 src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java bb6ab3b src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023 src/main/java/org/apache/hadoop/hbase/master/MXBean.java 7f44dc2 src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java 45b8fe7 src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java be63838 src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 80271b1 src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76 src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java efcf74d src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6 src/main/protobuf/RegionServerStatus.proto PRE-CREATION src/main/protobuf/hbase.proto 12e6053 src/main/resources/hbase-webapps/master/table.jsp 3ef1190 src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 72554cb src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java d039be3 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8 src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java bd5fa90 src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java e99d251 Diff: https://reviews.apache.org/r/4463/diff Testing ------- Ran jenkins job, all unit tests passed. Thanks, Gregory
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #191 (See https://builds.apache.org/job/HBase-TRUNK-security/191/)
        HBASE-5444 Add PB-based calls to HMasterRegionInterface (Revision 1333319)

        Result = SUCCESS
        stack :
        Files :

        • /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon
        • /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ClusterStatus.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ServerLoad.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MXBean.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/src/main/protobuf/RegionServerStatus.proto
        • /hbase/trunk/src/main/protobuf/hbase.proto
        • /hbase/trunk/src/main/resources/hbase-webapps/master/table.jsp
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #191 (See https://builds.apache.org/job/HBase-TRUNK-security/191/ ) HBASE-5444 Add PB-based calls to HMasterRegionInterface (Revision 1333319) Result = SUCCESS stack : Files : /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ClusterStatus.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ServerLoad.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HMasterRegionInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MXBean.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MXBeanImpl.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/MasterDumpServlet.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionServerStatusProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/protobuf/RegionServerStatus.proto /hbase/trunk/src/main/protobuf/hbase.proto /hbase/trunk/src/main/resources/hbase-webapps/master/table.jsp /hbase/trunk/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestClassLoading.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMXBean.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestServerCustomProtocol.java
        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.

          People

          • Assignee:
            Gregory Chanan
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development