HBase
  1. HBase
  2. HBASE-5443

Add PB-based calls to HRegionInterface

    Details

      Issue Links

        Activity

        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #182 (See https://builds.apache.org/job/HBase-TRUNK-security/182/)
        HBASE-5443 Convert admin protocol of HRegionInterface to PB (Revision 1329358)

        Result = SUCCESS
        stack :
        Files :

        • /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java
        • /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/SortedCopyOnWriteSet.java
        • /hbase/trunk/src/main/protobuf/Admin.proto
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #182 (See https://builds.apache.org/job/HBase-TRUNK-security/182/ ) HBASE-5443 Convert admin protocol of HRegionInterface to PB (Revision 1329358) Result = SUCCESS stack : Files : /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/SortedCopyOnWriteSet.java /hbase/trunk/src/main/protobuf/Admin.proto /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2799 (See https://builds.apache.org/job/HBase-TRUNK/2799/)
        HBASE-5443 Convert admin protocol of HRegionInterface to PB (Revision 1329358)

        Result = SUCCESS
        stack :
        Files :

        • /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java
        • /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/SortedCopyOnWriteSet.java
        • /hbase/trunk/src/main/protobuf/Admin.proto
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2799 (See https://builds.apache.org/job/HBase-TRUNK/2799/ ) HBASE-5443 Convert admin protocol of HRegionInterface to PB (Revision 1329358) Result = SUCCESS stack : Files : /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java /hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/SortedCopyOnWriteSet.java /hbase/trunk/src/main/protobuf/Admin.proto /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestHTableUtil.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #171 (See https://builds.apache.org/job/HBase-TRUNK-security/171/)
        HBASE-5443 Convert the client protocol of HRegionInterface to PB (Revision 1325937)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.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/RegionAdminProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java
        • /hbase/trunk/src/main/protobuf/Admin.proto
        • /hbase/trunk/src/main/protobuf/Client.proto
        • /hbase/trunk/src/main/protobuf/RegionAdmin.proto
        • /hbase/trunk/src/main/protobuf/RegionClient.proto
        • /hbase/trunk/src/main/protobuf/hbase.proto
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/OOMERegionServer.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #171 (See https://builds.apache.org/job/HBase-TRUNK-security/171/ ) HBASE-5443 Convert the client protocol of HRegionInterface to PB (Revision 1325937) Result = FAILURE stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.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/RegionAdminProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java /hbase/trunk/src/main/protobuf/Admin.proto /hbase/trunk/src/main/protobuf/Client.proto /hbase/trunk/src/main/protobuf/RegionAdmin.proto /hbase/trunk/src/main/protobuf/RegionClient.proto /hbase/trunk/src/main/protobuf/hbase.proto /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/OOMERegionServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2755 (See https://builds.apache.org/job/HBase-TRUNK/2755/)
        HBASE-5443 Convert the client protocol of HRegionInterface to PB (Revision 1325937)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.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/RegionAdminProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java
        • /hbase/trunk/src/main/protobuf/Admin.proto
        • /hbase/trunk/src/main/protobuf/Client.proto
        • /hbase/trunk/src/main/protobuf/RegionAdmin.proto
        • /hbase/trunk/src/main/protobuf/RegionClient.proto
        • /hbase/trunk/src/main/protobuf/hbase.proto
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/OOMERegionServer.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2755 (See https://builds.apache.org/job/HBase-TRUNK/2755/ ) HBASE-5443 Convert the client protocol of HRegionInterface to PB (Revision 1325937) Result = FAILURE stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.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/RegionAdminProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java /hbase/trunk/src/main/protobuf/Admin.proto /hbase/trunk/src/main/protobuf/Client.proto /hbase/trunk/src/main/protobuf/RegionAdmin.proto /hbase/trunk/src/main/protobuf/RegionClient.proto /hbase/trunk/src/main/protobuf/hbase.proto /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/OOMERegionServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
        Hide
        binlijin added a comment -

        @Jimmy Xiang and @stack ,
        Thank you very much!

        Show
        binlijin added a comment - @Jimmy Xiang and @stack , Thank you very much!
        Show
        stack added a comment - There is also this write up of Todd's on why pb in first place over in hdfs: https://issues.apache.org/jira/browse/HDFS-2058?focusedCommentId=13047289&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13047289
        Hide
        Jimmy Xiang added a comment -

        The main reason is that the HBase writable RPC already supports pb. Hadoop uses pb too.

        Show
        Jimmy Xiang added a comment - The main reason is that the HBase writable RPC already supports pb. Hadoop uses pb too.
        Hide
        binlijin added a comment -

        Hi guys,i have some question, why choose pb? why not avro or thrift?

        Show
        binlijin added a comment - Hi guys,i have some question, why choose pb? why not avro or thrift?
        Hide
        binlijin added a comment -

        Hi guys,i have some question, why choose pb? why not avro or thrift?

        Show
        binlijin added a comment - Hi guys,i have some question, why choose pb? why not avro or thrift?
        Hide
        binlijin added a comment -

        Hi guys,i have some question, why choose pb? why not avro or thrift?

        Show
        binlijin added a comment - Hi guys,i have some question, why choose pb? why not avro or thrift?
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #155 (See https://builds.apache.org/job/HBase-TRUNK-security/155/)
        HBASE-5443 Create PB protocols for HRegionInterface (Revision 1307625)

        Result = SUCCESS
        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/HRegionPartitioner.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionAdminProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/package.html
        • /hbase/trunk/src/main/protobuf
        • /hbase/trunk/src/main/protobuf/README.txt
        • /hbase/trunk/src/main/protobuf/RegionAdmin.proto
        • /hbase/trunk/src/main/protobuf/RegionClient.proto
        • /hbase/trunk/src/main/protobuf/hbase.proto
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #155 (See https://builds.apache.org/job/HBase-TRUNK-security/155/ ) HBASE-5443 Create PB protocols for HRegionInterface (Revision 1307625) Result = SUCCESS stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/HRegionPartitioner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionAdminProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/package.html /hbase/trunk/src/main/protobuf /hbase/trunk/src/main/protobuf/README.txt /hbase/trunk/src/main/protobuf/RegionAdmin.proto /hbase/trunk/src/main/protobuf/RegionClient.proto /hbase/trunk/src/main/protobuf/hbase.proto
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2701 (See https://builds.apache.org/job/HBase-TRUNK/2701/)
        HBASE-5443 Create PB protocols for HRegionInterface (Revision 1307625)

        Result = SUCCESS
        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/HRegionPartitioner.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionAdminProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/package.html
        • /hbase/trunk/src/main/protobuf
        • /hbase/trunk/src/main/protobuf/README.txt
        • /hbase/trunk/src/main/protobuf/RegionAdmin.proto
        • /hbase/trunk/src/main/protobuf/RegionClient.proto
        • /hbase/trunk/src/main/protobuf/hbase.proto
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2701 (See https://builds.apache.org/job/HBase-TRUNK/2701/ ) HBASE-5443 Create PB protocols for HRegionInterface (Revision 1307625) Result = SUCCESS stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/mapreduce/HRegionPartitioner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionAdminProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/package.html /hbase/trunk/src/main/protobuf /hbase/trunk/src/main/protobuf/README.txt /hbase/trunk/src/main/protobuf/RegionAdmin.proto /hbase/trunk/src/main/protobuf/RegionClient.proto /hbase/trunk/src/main/protobuf/hbase.proto
        Hide
        Jimmy Xiang added a comment -

        I have done some code changes, and some tests failed. It is very hard to look into them. So I'd like to break it into small pieces and tag them one by one.

        Show
        Jimmy Xiang added a comment - I have done some code changes, and some tests failed. It is very hard to look into them. So I'd like to break it into small pieces and tag them one by one.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > pom.xml, line 750

        > <https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line750>

        >

        > Do this instead:

        >

        > if which cygpath >/dev/null 2>/dev/null; then

        > # Windows

        > else

        > # Not Windows

        > fi

        Why do we need /dev/null twice?

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > pom.xml, line 761

        > <https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line761>

        >

        > Actually you can just remove the whole $IS_WIN business and everything. Simply fix PROTO_DIR and JAVA_DIR when on Windows before calling protoc.

        How about this? The reason for I_PROTO_DIR is to keep "ls $PROTO_DIR/*.proto" working.

        if which cygpath 2> /dev/null; then
        I_PROTO_DIR=$PROTO_DIR
        else
        I_PROTO_DIR=`cygpath --windows $PROTO_DIR`
        JAVA_DIR=`cygpath --windows $JAVA_DIR`
        fi
        mkdir -p $JAVA_DIR 2> /dev/null
        for PROTO_FILE in `ls $PROTO_DIR/*.proto 2> /dev/null`
        do
        protoc -I$I_PROTO_DIR --java_out=$JAVA_DIR $PROTO_FILE
        done

        • Jimmy

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

        On 2012-03-02 18:54:29, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-03-02 18:54:29)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml bb518b1

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

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-02 10:30:06, Benoit Sigoure wrote: > pom.xml, line 750 > < https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line750 > > > Do this instead: > > if which cygpath >/dev/null 2>/dev/null; then > # Windows > else > # Not Windows > fi Why do we need /dev/null twice? On 2012-03-02 10:30:06, Benoit Sigoure wrote: > pom.xml, line 761 > < https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line761 > > > Actually you can just remove the whole $IS_WIN business and everything. Simply fix PROTO_DIR and JAVA_DIR when on Windows before calling protoc. How about this? The reason for I_PROTO_DIR is to keep "ls $PROTO_DIR/*.proto" working. if which cygpath 2> /dev/null; then I_PROTO_DIR=$PROTO_DIR else I_PROTO_DIR=`cygpath --windows $PROTO_DIR` JAVA_DIR=`cygpath --windows $JAVA_DIR` fi mkdir -p $JAVA_DIR 2> /dev/null for PROTO_FILE in `ls $PROTO_DIR/*.proto 2> /dev/null` do protoc -I$I_PROTO_DIR --java_out=$JAVA_DIR $PROTO_FILE done Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5552 ----------------------------------------------------------- On 2012-03-02 18:54:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-02 18:54:29) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml bb518b1 src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-02 20:07:30, Gregory Chanan wrote:

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

        > <https://reviews.apache.org/r/4054/diff/2/?file=87680#file87680line23>

        >

        > You don't have "option optimize_for = SPEED;" here.

        will add.

        On 2012-03-02 20:07:30, Gregory Chanan wrote:

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

        > <https://reviews.apache.org/r/4054/diff/2/?file=87678#file87678line22>

        >

        > If we are getting rid of "Proto" in the message names, might as well get rid of it here too.

        This one, I am not sure?

        • Jimmy

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

        On 2012-03-02 18:54:29, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-03-02 18:54:29)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml bb518b1

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

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-02 20:07:30, Gregory Chanan wrote: > src/main/proto/hbase.proto, line 23 > < https://reviews.apache.org/r/4054/diff/2/?file=87680#file87680line23 > > > You don't have "option optimize_for = SPEED;" here. will add. On 2012-03-02 20:07:30, Gregory Chanan wrote: > src/main/proto/RegionAdmin.proto, line 22 > < https://reviews.apache.org/r/4054/diff/2/?file=87678#file87678line22 > > > If we are getting rid of "Proto" in the message names, might as well get rid of it here too. This one, I am not sure? Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5583 ----------------------------------------------------------- On 2012-03-02 18:54:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-02 18:54:29) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml bb518b1 src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        src/main/proto/RegionAdmin.proto
        <https://reviews.apache.org/r/4054/#comment12092>

        If we are getting rid of "Proto" in the message names, might as well get rid of it here too.

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment12094>

        Here too.

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment12095>

        Here too.

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment12091>

        You don't have "option optimize_for = SPEED;" here.

        • Gregory

        On 2012-03-02 18:54:29, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-03-02 18:54:29)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml bb518b1

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

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5583 ----------------------------------------------------------- src/main/proto/RegionAdmin.proto < https://reviews.apache.org/r/4054/#comment12092 > If we are getting rid of "Proto" in the message names, might as well get rid of it here too. src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment12094 > Here too. src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment12095 > Here too. src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment12091 > You don't have "option optimize_for = SPEED;" here. Gregory On 2012-03-02 18:54:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-02 18:54:29) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml bb518b1 src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-02 19:16:55, Benoit Sigoure wrote:

        > src/main/proto/RegionClient.proto, line 112

        > <https://reviews.apache.org/r/4054/diff/2/?file=87679#file87679line112>

        >

        > trailing whitespaces

        Will remove it.

        On 2012-03-02 19:16:55, Benoit Sigoure wrote:

        > pom.xml, line 764

        > <https://reviews.apache.org/r/4054/diff/2/?file=87677#file87677line764>

        >

        > You didn't take into account my comments on fixing this shell scripting from the previous iteration.

        Sorry. I forgot it. Good idea. I will address it in the next diff.

        On 2012-03-02 19:16:55, Benoit Sigoure wrote:

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

        > <https://reviews.apache.org/r/4054/diff/2/?file=87680#file87680line64>

        >

        > I still don't understand how these can be optional.

        Ok, I can make family and qualifier required.

        On 2012-03-02 19:16:55, Benoit Sigoure wrote:

        > src/main/proto/RegionClient.proto, line 147

        > <https://reviews.apache.org/r/4054/diff/2/?file=87679#file87679line147>

        >

        > I don't know if we should let the client specify the TTL. Right now in HBase the TTL is hardcoded in the Configuration object of the RegionServer.

        >

        > Actually I'm fine with allowing clients specify their own TTL as long as we bound the TTL with the servers' Configuration.

        I see. I will remove it. We can add it back if server supports it later on.

        How about lockRow? Does the server support client specified TTL?

        On 2012-03-02 19:16:55, Benoit Sigoure wrote:

        > src/main/proto/RegionClient.proto, line 63

        > <https://reviews.apache.org/r/4054/diff/2/?file=87679#file87679line63>

        >

        > So a Get request can only fetch multiple Get from a single Region? That's not good. We need true multi-get, where you can fetch things from multiple regions on the same RegionServer at once.

        I can move the region to Get. That means each Get need to specify a region, so it can be duplicated. Another option is to add another message like GetGroup which has a region and a set of Gets.

        One more option is to make region optional in the request, and add an optional region to Get. The region in GetRequest will be used if there is no region specified in Get.

        What do you think?

        • Jimmy

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

        On 2012-03-02 18:54:29, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-03-02 18:54:29)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml bb518b1

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

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-02 19:16:55, Benoit Sigoure wrote: > src/main/proto/RegionClient.proto, line 112 > < https://reviews.apache.org/r/4054/diff/2/?file=87679#file87679line112 > > > trailing whitespaces Will remove it. On 2012-03-02 19:16:55, Benoit Sigoure wrote: > pom.xml, line 764 > < https://reviews.apache.org/r/4054/diff/2/?file=87677#file87677line764 > > > You didn't take into account my comments on fixing this shell scripting from the previous iteration. Sorry. I forgot it. Good idea. I will address it in the next diff. On 2012-03-02 19:16:55, Benoit Sigoure wrote: > src/main/proto/hbase.proto, line 64 > < https://reviews.apache.org/r/4054/diff/2/?file=87680#file87680line64 > > > I still don't understand how these can be optional. Ok, I can make family and qualifier required. On 2012-03-02 19:16:55, Benoit Sigoure wrote: > src/main/proto/RegionClient.proto, line 147 > < https://reviews.apache.org/r/4054/diff/2/?file=87679#file87679line147 > > > I don't know if we should let the client specify the TTL. Right now in HBase the TTL is hardcoded in the Configuration object of the RegionServer. > > Actually I'm fine with allowing clients specify their own TTL as long as we bound the TTL with the servers' Configuration. I see. I will remove it. We can add it back if server supports it later on. How about lockRow? Does the server support client specified TTL? On 2012-03-02 19:16:55, Benoit Sigoure wrote: > src/main/proto/RegionClient.proto, line 63 > < https://reviews.apache.org/r/4054/diff/2/?file=87679#file87679line63 > > > So a Get request can only fetch multiple Get from a single Region? That's not good. We need true multi-get, where you can fetch things from multiple regions on the same RegionServer at once. I can move the region to Get. That means each Get need to specify a region, so it can be duplicated. Another option is to add another message like GetGroup which has a region and a set of Gets. One more option is to make region optional in the request, and add an optional region to Get. The region in GetRequest will be used if there is no region specified in Get. What do you think? Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5573 ----------------------------------------------------------- On 2012-03-02 18:54:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-02 18:54:29) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml bb518b1 src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-02 19:31:46, Jimmy Xiang wrote:

        > This is another option for scan. This way, we will have only one scan method, no need to open/next/close.

        >

        > Which one do you prefer? In the ScanRequest, either scannerId or scan must be specified, not both.

        >

        > message Scan { bq. > required RegionSpecifier region = 1; bq. > repeated Column column = 2; bq. > repeated Attribute attribute = 3; bq. > optional bytes startRow = 4; bq. > optional bytes stopRow = 5; bq. > optional string filterName = 6; bq. > optional TimeRange timeRange = 7; bq. > optional uint32 maxVersions = 8 [default = 1]; bq. > optional bool cacheBlocks = 9 [default = true]; bq. > optional uint32 rowsToCache = 10; bq. > optional uint32 batchSize = 11; bq. > }

        >

        > message ScanRequest { bq. > optional uint64 scannerId = 1; bq. > optional Scan scan = 2; bq. > optional uint32 numberOfRows = 3; bq. > optional bool closeScanner = 4; bq. > optional uint32 ttl = 5; bq. > }

        >

        > message ScanResponse { bq. > repeated Result result = 1; bq. > optional uint64 scannerId = 2; bq. > optional bool moreResults = 3; bq. > optional uint32 ttl = 4; bq. > }

        >

        Michael Stack wrote:

        So we would do away with openScanner, next, and close, just do scan? Inside in the ScanRequest, we'd carry over the Scan specification each time? We'd be able to honor the current openScanner, next, close client-facing API but could add a new scan method to the public api that allowed passing the above specifications? Sounds good.

        The only issue is that both optional. They need to know to specify one. From documentation?

        • Jimmy

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

        On 2012-03-02 18:54:29, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-03-02 18:54:29)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml bb518b1

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

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-02 19:31:46, Jimmy Xiang wrote: > This is another option for scan. This way, we will have only one scan method, no need to open/next/close. > > Which one do you prefer? In the ScanRequest, either scannerId or scan must be specified, not both. > > message Scan { bq. > required RegionSpecifier region = 1; bq. > repeated Column column = 2; bq. > repeated Attribute attribute = 3; bq. > optional bytes startRow = 4; bq. > optional bytes stopRow = 5; bq. > optional string filterName = 6; bq. > optional TimeRange timeRange = 7; bq. > optional uint32 maxVersions = 8 [default = 1]; bq. > optional bool cacheBlocks = 9 [default = true]; bq. > optional uint32 rowsToCache = 10; bq. > optional uint32 batchSize = 11; bq. > } > > message ScanRequest { bq. > optional uint64 scannerId = 1; bq. > optional Scan scan = 2; bq. > optional uint32 numberOfRows = 3; bq. > optional bool closeScanner = 4; bq. > optional uint32 ttl = 5; bq. > } > > message ScanResponse { bq. > repeated Result result = 1; bq. > optional uint64 scannerId = 2; bq. > optional bool moreResults = 3; bq. > optional uint32 ttl = 4; bq. > } > Michael Stack wrote: So we would do away with openScanner, next, and close, just do scan? Inside in the ScanRequest, we'd carry over the Scan specification each time? We'd be able to honor the current openScanner, next, close client-facing API but could add a new scan method to the public api that allowed passing the above specifications? Sounds good. The only issue is that both optional. They need to know to specify one. From documentation? Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5575 ----------------------------------------------------------- On 2012-03-02 18:54:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-02 18:54:29) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml bb518b1 src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-02 19:31:46, Jimmy Xiang wrote:

        > This is another option for scan. This way, we will have only one scan method, no need to open/next/close.

        >

        > Which one do you prefer? In the ScanRequest, either scannerId or scan must be specified, not both.

        >

        > message Scan { bq. > required RegionSpecifier region = 1; bq. > repeated Column column = 2; bq. > repeated Attribute attribute = 3; bq. > optional bytes startRow = 4; bq. > optional bytes stopRow = 5; bq. > optional string filterName = 6; bq. > optional TimeRange timeRange = 7; bq. > optional uint32 maxVersions = 8 [default = 1]; bq. > optional bool cacheBlocks = 9 [default = true]; bq. > optional uint32 rowsToCache = 10; bq. > optional uint32 batchSize = 11; bq. > }

        >

        > message ScanRequest { bq. > optional uint64 scannerId = 1; bq. > optional Scan scan = 2; bq. > optional uint32 numberOfRows = 3; bq. > optional bool closeScanner = 4; bq. > optional uint32 ttl = 5; bq. > }

        >

        > message ScanResponse { bq. > repeated Result result = 1; bq. > optional uint64 scannerId = 2; bq. > optional bool moreResults = 3; bq. > optional uint32 ttl = 4; bq. > }

        >

        So we would do away with openScanner, next, and close, just do scan? Inside in the ScanRequest, we'd carry over the Scan specification each time? We'd be able to honor the current openScanner, next, close client-facing API but could add a new scan method to the public api that allowed passing the above specifications? Sounds good.

        • Michael

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

        On 2012-03-02 18:54:29, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-03-02 18:54:29)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml bb518b1

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

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-02 19:31:46, Jimmy Xiang wrote: > This is another option for scan. This way, we will have only one scan method, no need to open/next/close. > > Which one do you prefer? In the ScanRequest, either scannerId or scan must be specified, not both. > > message Scan { bq. > required RegionSpecifier region = 1; bq. > repeated Column column = 2; bq. > repeated Attribute attribute = 3; bq. > optional bytes startRow = 4; bq. > optional bytes stopRow = 5; bq. > optional string filterName = 6; bq. > optional TimeRange timeRange = 7; bq. > optional uint32 maxVersions = 8 [default = 1]; bq. > optional bool cacheBlocks = 9 [default = true]; bq. > optional uint32 rowsToCache = 10; bq. > optional uint32 batchSize = 11; bq. > } > > message ScanRequest { bq. > optional uint64 scannerId = 1; bq. > optional Scan scan = 2; bq. > optional uint32 numberOfRows = 3; bq. > optional bool closeScanner = 4; bq. > optional uint32 ttl = 5; bq. > } > > message ScanResponse { bq. > repeated Result result = 1; bq. > optional uint64 scannerId = 2; bq. > optional bool moreResults = 3; bq. > optional uint32 ttl = 4; bq. > } > So we would do away with openScanner, next, and close, just do scan? Inside in the ScanRequest, we'd carry over the Scan specification each time? We'd be able to honor the current openScanner, next, close client-facing API but could add a new scan method to the public api that allowed passing the above specifications? Sounds good. Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5575 ----------------------------------------------------------- On 2012-03-02 18:54:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-02 18:54:29) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml bb518b1 src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        This is another option for scan. This way, we will have only one scan method, no need to open/next/close.

        Which one do you prefer? In the ScanRequest, either scannerId or scan must be specified, not both.

        message Scan

        { required RegionSpecifier region = 1; repeated Column column = 2; repeated Attribute attribute = 3; optional bytes startRow = 4; optional bytes stopRow = 5; optional string filterName = 6; optional TimeRange timeRange = 7; optional uint32 maxVersions = 8 [default = 1]; optional bool cacheBlocks = 9 [default = true]; optional uint32 rowsToCache = 10; optional uint32 batchSize = 11; }

        message ScanRequest

        { optional uint64 scannerId = 1; optional Scan scan = 2; optional uint32 numberOfRows = 3; optional bool closeScanner = 4; optional uint32 ttl = 5; }

        message ScanResponse

        { repeated Result result = 1; optional uint64 scannerId = 2; optional bool moreResults = 3; optional uint32 ttl = 4; }
        • Jimmy

        On 2012-03-02 18:54:29, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-03-02 18:54:29)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml bb518b1

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

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5575 ----------------------------------------------------------- This is another option for scan. This way, we will have only one scan method, no need to open/next/close. Which one do you prefer? In the ScanRequest, either scannerId or scan must be specified, not both. message Scan { required RegionSpecifier region = 1; repeated Column column = 2; repeated Attribute attribute = 3; optional bytes startRow = 4; optional bytes stopRow = 5; optional string filterName = 6; optional TimeRange timeRange = 7; optional uint32 maxVersions = 8 [default = 1]; optional bool cacheBlocks = 9 [default = true]; optional uint32 rowsToCache = 10; optional uint32 batchSize = 11; } message ScanRequest { optional uint64 scannerId = 1; optional Scan scan = 2; optional uint32 numberOfRows = 3; optional bool closeScanner = 4; optional uint32 ttl = 5; } message ScanResponse { repeated Result result = 1; optional uint64 scannerId = 2; optional bool moreResults = 3; optional uint32 ttl = 4; } Jimmy On 2012-03-02 18:54:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-02 18:54:29) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml bb518b1 src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        This is a lot better already. One thing this doesn't address that I should've mentioned in my previous review is that the requests and responses still have a lot of duplicate data. For example if I "Get" a row that contains 3 KeyValue, in the response, on the wire, I'll get 3 times the key and 3 times the family.

        pom.xml
        <https://reviews.apache.org/r/4054/#comment12066>

        You didn't take into account my comments on fixing this shell scripting from the previous iteration.

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment12069>

        So a Get request can only fetch multiple Get from a single Region? That's not good. We need true multi-get, where you can fetch things from multiple regions on the same RegionServer at once.

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment12068>

        trailing whitespaces

        src/main/proto/RegionClient.proto
        <https://reviews.apache.org/r/4054/#comment12070>

        I don't know if we should let the client specify the TTL. Right now in HBase the TTL is hardcoded in the Configuration object of the RegionServer.

        Actually I'm fine with allowing clients specify their own TTL as long as we bound the TTL with the servers' Configuration.

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment12071>

        I still don't understand how these can be optional.

        • Benoit

        On 2012-03-02 18:54:29, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-03-02 18:54:29)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml bb518b1

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

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5573 ----------------------------------------------------------- This is a lot better already. One thing this doesn't address that I should've mentioned in my previous review is that the requests and responses still have a lot of duplicate data. For example if I "Get" a row that contains 3 KeyValue, in the response, on the wire, I'll get 3 times the key and 3 times the family. pom.xml < https://reviews.apache.org/r/4054/#comment12066 > You didn't take into account my comments on fixing this shell scripting from the previous iteration. src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment12069 > So a Get request can only fetch multiple Get from a single Region? That's not good. We need true multi-get, where you can fetch things from multiple regions on the same RegionServer at once. src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment12068 > trailing whitespaces src/main/proto/RegionClient.proto < https://reviews.apache.org/r/4054/#comment12070 > I don't know if we should let the client specify the TTL. Right now in HBase the TTL is hardcoded in the Configuration object of the RegionServer. Actually I'm fine with allowing clients specify their own TTL as long as we bound the TTL with the servers' Configuration. src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment12071 > I still don't understand how these can be optional. Benoit On 2012-03-02 18:54:29, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-02 18:54:29) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml bb518b1 src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        Jimmy Xiang added a comment -

        I updated the review with new diff, which incorporated the feedbacks from all reviewers. Thanks a lot for review.

        Show
        Jimmy Xiang added a comment - I updated the review with new diff, which incorporated the feedbacks from all reviewers. Thanks a lot for review.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-03-02 18:54:29.710858)

        Review request for hbase.

        Summary
        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

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

        Diffs


        pom.xml bb518b1
        src/main/proto/RegionAdmin.proto PRE-CREATION
        src/main/proto/RegionClient.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION

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

        Testing
        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-03-02 18:54:29.710858) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs pom.xml bb518b1 src/main/proto/RegionAdmin.proto PRE-CREATION src/main/proto/RegionClient.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > This seems to be close to a one-to-one mapping with the current interface today. I don't know if this is the intent or whether you're willing to completely redesign the look of the API too. Maybe it's to ease the transition?

        >

        > I'd like to see a request type to do one-shot scans. Something where you don't even get a scanner ID. You pass parameters like to open a scanner, you say up to how many rows or bytes you want to retrieve, and you get just that in one shot.

        > When opening a actual scanner, we also need to be able to get the first batch of scan results at the same time we open the scanner. This is a must-have IMO. And we need to be able to request to close the scanner while fetching a batch of results.

        >

        > It would be nice to have a "keep-alive" request for existing scanners. Something to tell the server "I'm not fetching anything from this scanner right now, but please keep it open by reseting its TTL, don't close it just because I haven't used it for a while".

        >

        > Please, please, please, consider shortening the name of all these protobufs and dropping the Proto suffix. The current names are unnecessarily long or aren't intuitive (e.g. "columnFamily" for something that describes the multiple things you're trying to get out of a row) or are too redundant (e.g. "KeyType keyType").

        >

        > Regarding the lack of "multi" RPC, I think this is a good thing. "multi" is a big mess that was only marginally better than its horrible "multiPut" predecessor. This proposal already supports multi-everything, it just doesn't support batching different kind of operations in the same RPC, which isn't a big deal IMO.

        Michael Stack wrote:

        We should implement what Benoît is asking for, probably not all as part of this issue. That said, if possible can we try and accomodate what he's asking for down here at the rpc level? I suppose once all is pb, it should be easy enough adding new stuff but it would be good to keep in mind what he's asking while redoing this layer. In a later issue we can add the overloads that exploit the additions or add the new methods B wants (What B is asking for are long-time outstanding fixups needed in hbase). For example, can the pb response on open of a scanner be more than just the scanner id; could it include an optional result item? Or I suppose, once up on pb, we can do this easily enough later?

        The idea is not to break the existing client application code. So the new interface should be able to do the same thing and more.
        By the way, I have changed the interfaces a lot after several reviews so I closed this review. I will post a new review later.

        As to scanner, we cannot retrieve everything in one shot. So in the RPC layer, there must be multiple trips. As to the function you mentioned, it can be built in the client side, right?
        I will add an option to return results in opening a scanner, and an option to close the scanner in fetching from the scanner.

        Ok, I will try to shorten the names.

        As to multi, I am not sure. This proposal doesn't support mix different kind of operations in different order in the same RPC. I may need to add a similar one if we don't want to break the existing function.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line22>

        >

        > I find the Proto suffix unnecessary and long. If you truly want a suffix, PB would be shorter, but no suffix would be better IMO.

        Ok, I will remove the Proto suffix.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > src/main/proto/HRegionProtocol.proto, line 25

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line25>

        >

        > Use "option optimize_for = SPEED", it makes a big difference.

        Cool. I will add it.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > src/main/proto/HRegionProtocol.proto, line 28

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line28>

        >

        > I'd call this just "Columns".

        I changed it to ColumnProto.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line30>

        >

        > I would recommend to pluralize all "repeated" fields. This will make for nicer code where you'll be able to write something along the lines of:

        >

        > for (byte[] qualifier : pb.qualifiers())

        For the repeated fields, the generated code will have method addQualifier(index), getQualifierList(). I don't think we need pluralization.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > src/main/proto/HRegionProtocol.proto, line 88

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line88>

        >

        > The thing that append() and increment() have in common is that they're atomic operations that don't require a read-modify-write from the client. So maybe AtomicOp would be better?

        In my new one, I have an optional parameter to indicate if atomic operations requested, which is for a set of mutations.
        One mutation such as append() and increment() should always be atomic, right?

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > src/main/proto/HRegionProtocol.proto, line 192

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line192>

        >

        > What's the meaning of this? How do we know what has been processed and what hasn't?

        I removed it.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > src/main/proto/HRegionProtocol.proto, line 221

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line221>

        >

        > We need to have a way to get feedback from the server about the TTL of the scanner. How long can the client hold on to the scanner before the server will kill it. Add a field here so that the server can communicate the TTL to the client.

        Sure. Will add it.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > src/main/proto/HRegionProtocol.proto, line 226

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line226>

        >

        > Please add an "optional boolean close" to request that the scanner be closed after returning this batch of results. This can help clients eliminate the "CloseScannerRequestProto" when they know they're going to close the scanner after this batch.

        Sure, will do.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > src/main/proto/HRegionProtocol.proto, line 269

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line269>

        >

        > This name is inconsistent with the name of the request. By your scheme it should be named "LockRowResponseProto" – although I'd much prefer "LockResp" or something like that.

        How about LockRowResponse?

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > src/main/proto/HRegionProtocol.proto, line 271

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line271>

        >

        > This needs to have a field specifying how long the server is willing to hand out the lock for.

        Ok, will add one.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line36>

        >

        > Call these fields just "from" and "to".

        Ok.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line60>

        >

        > Why are all these fields optional? How can a KeyValue not have a family or a qualifier or a timestamp?

        In case later on we replace the existing KeyValue class with this one.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line62>

        >

        > I'd recommend naming this "timestamp" not "time".

        ok.

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line63>

        >

        > Just call this "type".

        If later on, we add another type, such as ValueType, it will be confusing, right?

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-02 10:30:06, Benoit Sigoure wrote: > This seems to be close to a one-to-one mapping with the current interface today. I don't know if this is the intent or whether you're willing to completely redesign the look of the API too. Maybe it's to ease the transition? > > I'd like to see a request type to do one-shot scans. Something where you don't even get a scanner ID. You pass parameters like to open a scanner, you say up to how many rows or bytes you want to retrieve, and you get just that in one shot. > When opening a actual scanner, we also need to be able to get the first batch of scan results at the same time we open the scanner. This is a must-have IMO. And we need to be able to request to close the scanner while fetching a batch of results. > > It would be nice to have a "keep-alive" request for existing scanners. Something to tell the server "I'm not fetching anything from this scanner right now, but please keep it open by reseting its TTL, don't close it just because I haven't used it for a while". > > Please, please, please, consider shortening the name of all these protobufs and dropping the Proto suffix. The current names are unnecessarily long or aren't intuitive (e.g. "columnFamily" for something that describes the multiple things you're trying to get out of a row) or are too redundant (e.g. "KeyType keyType"). > > Regarding the lack of "multi" RPC, I think this is a good thing. "multi" is a big mess that was only marginally better than its horrible "multiPut" predecessor. This proposal already supports multi-everything, it just doesn't support batching different kind of operations in the same RPC, which isn't a big deal IMO. Michael Stack wrote: We should implement what Benoît is asking for, probably not all as part of this issue. That said, if possible can we try and accomodate what he's asking for down here at the rpc level? I suppose once all is pb, it should be easy enough adding new stuff but it would be good to keep in mind what he's asking while redoing this layer. In a later issue we can add the overloads that exploit the additions or add the new methods B wants (What B is asking for are long-time outstanding fixups needed in hbase). For example, can the pb response on open of a scanner be more than just the scanner id; could it include an optional result item? Or I suppose, once up on pb, we can do this easily enough later? The idea is not to break the existing client application code. So the new interface should be able to do the same thing and more. By the way, I have changed the interfaces a lot after several reviews so I closed this review. I will post a new review later. As to scanner, we cannot retrieve everything in one shot. So in the RPC layer, there must be multiple trips. As to the function you mentioned, it can be built in the client side, right? I will add an option to return results in opening a scanner, and an option to close the scanner in fetching from the scanner. Ok, I will try to shorten the names. As to multi, I am not sure. This proposal doesn't support mix different kind of operations in different order in the same RPC. I may need to add a similar one if we don't want to break the existing function. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/HRegionProtocol.proto, line 22 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line22 > > > I find the Proto suffix unnecessary and long. If you truly want a suffix, PB would be shorter, but no suffix would be better IMO. Ok, I will remove the Proto suffix. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/HRegionProtocol.proto, line 25 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line25 > > > Use "option optimize_for = SPEED", it makes a big difference. Cool. I will add it. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/HRegionProtocol.proto, line 28 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line28 > > > I'd call this just "Columns". I changed it to ColumnProto. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/HRegionProtocol.proto, line 30 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line30 > > > I would recommend to pluralize all "repeated" fields. This will make for nicer code where you'll be able to write something along the lines of: > > for (byte[] qualifier : pb.qualifiers()) For the repeated fields, the generated code will have method addQualifier(index), getQualifierList(). I don't think we need pluralization. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/HRegionProtocol.proto, line 88 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line88 > > > The thing that append() and increment() have in common is that they're atomic operations that don't require a read-modify-write from the client. So maybe AtomicOp would be better? In my new one, I have an optional parameter to indicate if atomic operations requested, which is for a set of mutations. One mutation such as append() and increment() should always be atomic, right? On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/HRegionProtocol.proto, line 192 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line192 > > > What's the meaning of this? How do we know what has been processed and what hasn't? I removed it. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/HRegionProtocol.proto, line 221 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line221 > > > We need to have a way to get feedback from the server about the TTL of the scanner. How long can the client hold on to the scanner before the server will kill it. Add a field here so that the server can communicate the TTL to the client. Sure. Will add it. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/HRegionProtocol.proto, line 226 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line226 > > > Please add an "optional boolean close" to request that the scanner be closed after returning this batch of results. This can help clients eliminate the "CloseScannerRequestProto" when they know they're going to close the scanner after this batch. Sure, will do. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/HRegionProtocol.proto, line 269 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line269 > > > This name is inconsistent with the name of the request. By your scheme it should be named "LockRowResponseProto" – although I'd much prefer "LockResp" or something like that. How about LockRowResponse? On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/HRegionProtocol.proto, line 271 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line271 > > > This needs to have a field specifying how long the server is willing to hand out the lock for. Ok, will add one. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/hbase.proto, line 36 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line36 > > > Call these fields just "from" and "to". Ok. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/hbase.proto, line 60 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line60 > > > Why are all these fields optional? How can a KeyValue not have a family or a qualifier or a timestamp? In case later on we replace the existing KeyValue class with this one. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/hbase.proto, line 62 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line62 > > > I'd recommend naming this "timestamp" not "time". ok. On 2012-03-02 10:30:06, Benoit Sigoure wrote: > src/main/proto/hbase.proto, line 63 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line63 > > > Just call this "type". If later on, we add another type, such as ValueType, it will be confusing, right? Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5552 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-02 10:30:06, Benoit Sigoure wrote:

        > This seems to be close to a one-to-one mapping with the current interface today. I don't know if this is the intent or whether you're willing to completely redesign the look of the API too. Maybe it's to ease the transition?

        >

        > I'd like to see a request type to do one-shot scans. Something where you don't even get a scanner ID. You pass parameters like to open a scanner, you say up to how many rows or bytes you want to retrieve, and you get just that in one shot.

        > When opening a actual scanner, we also need to be able to get the first batch of scan results at the same time we open the scanner. This is a must-have IMO. And we need to be able to request to close the scanner while fetching a batch of results.

        >

        > It would be nice to have a "keep-alive" request for existing scanners. Something to tell the server "I'm not fetching anything from this scanner right now, but please keep it open by reseting its TTL, don't close it just because I haven't used it for a while".

        >

        > Please, please, please, consider shortening the name of all these protobufs and dropping the Proto suffix. The current names are unnecessarily long or aren't intuitive (e.g. "columnFamily" for something that describes the multiple things you're trying to get out of a row) or are too redundant (e.g. "KeyType keyType").

        >

        > Regarding the lack of "multi" RPC, I think this is a good thing. "multi" is a big mess that was only marginally better than its horrible "multiPut" predecessor. This proposal already supports multi-everything, it just doesn't support batching different kind of operations in the same RPC, which isn't a big deal IMO.

        We should implement what Benoît is asking for, probably not all as part of this issue. That said, if possible can we try and accomodate what he's asking for down here at the rpc level? I suppose once all is pb, it should be easy enough adding new stuff but it would be good to keep in mind what he's asking while redoing this layer. In a later issue we can add the overloads that exploit the additions or add the new methods B wants (What B is asking for are long-time outstanding fixups needed in hbase). For example, can the pb response on open of a scanner be more than just the scanner id; could it include an optional result item? Or I suppose, once up on pb, we can do this easily enough later?

        • Michael

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-02 10:30:06, Benoit Sigoure wrote: > This seems to be close to a one-to-one mapping with the current interface today. I don't know if this is the intent or whether you're willing to completely redesign the look of the API too. Maybe it's to ease the transition? > > I'd like to see a request type to do one-shot scans. Something where you don't even get a scanner ID. You pass parameters like to open a scanner, you say up to how many rows or bytes you want to retrieve, and you get just that in one shot. > When opening a actual scanner, we also need to be able to get the first batch of scan results at the same time we open the scanner. This is a must-have IMO. And we need to be able to request to close the scanner while fetching a batch of results. > > It would be nice to have a "keep-alive" request for existing scanners. Something to tell the server "I'm not fetching anything from this scanner right now, but please keep it open by reseting its TTL, don't close it just because I haven't used it for a while". > > Please, please, please, consider shortening the name of all these protobufs and dropping the Proto suffix. The current names are unnecessarily long or aren't intuitive (e.g. "columnFamily" for something that describes the multiple things you're trying to get out of a row) or are too redundant (e.g. "KeyType keyType"). > > Regarding the lack of "multi" RPC, I think this is a good thing. "multi" is a big mess that was only marginally better than its horrible "multiPut" predecessor. This proposal already supports multi-everything, it just doesn't support batching different kind of operations in the same RPC, which isn't a big deal IMO. We should implement what Benoît is asking for, probably not all as part of this issue. That said, if possible can we try and accomodate what he's asking for down here at the rpc level? I suppose once all is pb, it should be easy enough adding new stuff but it would be good to keep in mind what he's asking while redoing this layer. In a later issue we can add the overloads that exploit the additions or add the new methods B wants (What B is asking for are long-time outstanding fixups needed in hbase). For example, can the pb response on open of a scanner be more than just the scanner id; could it include an optional result item? Or I suppose, once up on pb, we can do this easily enough later? Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5552 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        This seems to be close to a one-to-one mapping with the current interface today. I don't know if this is the intent or whether you're willing to completely redesign the look of the API too. Maybe it's to ease the transition?

        I'd like to see a request type to do one-shot scans. Something where you don't even get a scanner ID. You pass parameters like to open a scanner, you say up to how many rows or bytes you want to retrieve, and you get just that in one shot.
        When opening a actual scanner, we also need to be able to get the first batch of scan results at the same time we open the scanner. This is a must-have IMO. And we need to be able to request to close the scanner while fetching a batch of results.

        It would be nice to have a "keep-alive" request for existing scanners. Something to tell the server "I'm not fetching anything from this scanner right now, but please keep it open by reseting its TTL, don't close it just because I haven't used it for a while".

        Please, please, please, consider shortening the name of all these protobufs and dropping the Proto suffix. The current names are unnecessarily long or aren't intuitive (e.g. "columnFamily" for something that describes the multiple things you're trying to get out of a row) or are too redundant (e.g. "KeyType keyType").

        Regarding the lack of "multi" RPC, I think this is a good thing. "multi" is a big mess that was only marginally better than its horrible "multiPut" predecessor. This proposal already supports multi-everything, it just doesn't support batching different kind of operations in the same RPC, which isn't a big deal IMO.

        pom.xml
        <https://reviews.apache.org/r/4054/#comment11997>

        Do this instead:

        if which cygpath >/dev/null 2>/dev/null; then

        1. Windows
          else
        2. Not Windows
          fi

        pom.xml
        <https://reviews.apache.org/r/4054/#comment11998>

        Simply do:

        if $IS_WIN; then

        pom.xml
        <https://reviews.apache.org/r/4054/#comment12016>

        Actually you can just remove the whole $IS_WIN business and everything. Simply fix PROTO_DIR and JAVA_DIR when on Windows before calling protoc.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11999>

        I find the Proto suffix unnecessary and long. If you truly want a suffix, PB would be shorter, but no suffix would be better IMO.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12004>

        Use "option optimize_for = SPEED", it makes a big difference.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12006>

        I'd call this just "Columns".

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12005>

        I would recommend to pluralize all "repeated" fields. This will make for nicer code where you'll be able to write something along the lines of:

        for (byte[] qualifier : pb.qualifiers())

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12010>

        The thing that append() and increment() have in common is that they're atomic operations that don't require a read-modify-write from the client. So maybe AtomicOp would be better?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12012>

        Just call this Columns.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12019>

        What's the meaning of this? How do we know what has been processed and what hasn't?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12000>

        I'd vote for adding this right now. It's easy to add directly and would be a huge improvement for short scans (which are super common).

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12007>

        We need to have a way to get feedback from the server about the TTL of the scanner. How long can the client hold on to the scanner before the server will kill it. Add a field here so that the server can communicate the TTL to the client.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12001>

        Please add an "optional boolean close" to request that the scanner be closed after returning this batch of results. This can help clients eliminate the "CloseScannerRequestProto" when they know they're going to close the scanner after this batch.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12017>

        This name is inconsistent with the name of the request. By your scheme it should be named "LockRowResponseProto" – although I'd much prefer "LockResp" or something like that.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment12018>

        This needs to have a field specifying how long the server is willing to hand out the lock for.

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment12015>

        Call these fields just "from" and "to".

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment12002>

        Why are all these fields optional? How can a KeyValue not have a family or a qualifier or a timestamp?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment12003>

        I'd recommend naming this "timestamp" not "time".

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment12013>

        Just call this "type".

        • Benoit

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5552 ----------------------------------------------------------- This seems to be close to a one-to-one mapping with the current interface today. I don't know if this is the intent or whether you're willing to completely redesign the look of the API too. Maybe it's to ease the transition? I'd like to see a request type to do one-shot scans. Something where you don't even get a scanner ID. You pass parameters like to open a scanner, you say up to how many rows or bytes you want to retrieve, and you get just that in one shot. When opening a actual scanner, we also need to be able to get the first batch of scan results at the same time we open the scanner. This is a must-have IMO. And we need to be able to request to close the scanner while fetching a batch of results. It would be nice to have a "keep-alive" request for existing scanners. Something to tell the server "I'm not fetching anything from this scanner right now, but please keep it open by reseting its TTL, don't close it just because I haven't used it for a while". Please, please, please, consider shortening the name of all these protobufs and dropping the Proto suffix. The current names are unnecessarily long or aren't intuitive (e.g. "columnFamily" for something that describes the multiple things you're trying to get out of a row) or are too redundant (e.g. "KeyType keyType"). Regarding the lack of "multi" RPC, I think this is a good thing. "multi" is a big mess that was only marginally better than its horrible "multiPut" predecessor. This proposal already supports multi-everything, it just doesn't support batching different kind of operations in the same RPC, which isn't a big deal IMO. pom.xml < https://reviews.apache.org/r/4054/#comment11997 > Do this instead: if which cygpath >/dev/null 2>/dev/null; then Windows else Not Windows fi pom.xml < https://reviews.apache.org/r/4054/#comment11998 > Simply do: if $IS_WIN; then pom.xml < https://reviews.apache.org/r/4054/#comment12016 > Actually you can just remove the whole $IS_WIN business and everything. Simply fix PROTO_DIR and JAVA_DIR when on Windows before calling protoc. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11999 > I find the Proto suffix unnecessary and long. If you truly want a suffix, PB would be shorter, but no suffix would be better IMO. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12004 > Use "option optimize_for = SPEED", it makes a big difference. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12006 > I'd call this just "Columns". src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12005 > I would recommend to pluralize all "repeated" fields. This will make for nicer code where you'll be able to write something along the lines of: for (byte[] qualifier : pb.qualifiers()) src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12010 > The thing that append() and increment() have in common is that they're atomic operations that don't require a read-modify-write from the client. So maybe AtomicOp would be better? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12012 > Just call this Columns. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12019 > What's the meaning of this? How do we know what has been processed and what hasn't? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12000 > I'd vote for adding this right now. It's easy to add directly and would be a huge improvement for short scans (which are super common). src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12007 > We need to have a way to get feedback from the server about the TTL of the scanner. How long can the client hold on to the scanner before the server will kill it. Add a field here so that the server can communicate the TTL to the client. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12001 > Please add an "optional boolean close" to request that the scanner be closed after returning this batch of results. This can help clients eliminate the "CloseScannerRequestProto" when they know they're going to close the scanner after this batch. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12017 > This name is inconsistent with the name of the request. By your scheme it should be named "LockRowResponseProto" – although I'd much prefer "LockResp" or something like that. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment12018 > This needs to have a field specifying how long the server is willing to hand out the lock for. src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment12015 > Call these fields just "from" and "to". src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment12002 > Why are all these fields optional? How can a KeyValue not have a family or a qualifier or a timestamp? src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment12003 > I'd recommend naming this "timestamp" not "time". src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment12013 > Just call this "type". Benoit On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-01 19:35:08, Michael Stack wrote:

        > pom.xml, line 746

        > <https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line746>

        >

        > So, all proto generated files go to proto/generated? All into the same package? Thanks Jimmy.

        >

        > Also, mind checking out Deveraj's patch? I'd suggest at least reviewing it to figure if you fellas are using same conventions going all proto.

        >

        > Good stuff

        I will change mine to protobuf/generated. Thrift is using protobuf.
        I posted a comment on Deveraj's patch to ask if he can use sub-folder "generated".

        Thanks a lot for review. I updated my patch per these comments.
        I will post a different review once I am ready.

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-01 19:35:08, Michael Stack wrote: > pom.xml, line 746 > < https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line746 > > > So, all proto generated files go to proto/generated? All into the same package? Thanks Jimmy. > > Also, mind checking out Deveraj's patch? I'd suggest at least reviewing it to figure if you fellas are using same conventions going all proto. > > Good stuff I will change mine to protobuf/generated. Thrift is using protobuf. I posted a comment on Deveraj's patch to ask if he can use sub-folder "generated". Thanks a lot for review. I updated my patch per these comments. I will post a different review once I am ready. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5511 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        pom.xml
        <https://reviews.apache.org/r/4054/#comment11930>

        So, all proto generated files go to proto/generated? All into the same package? Thanks Jimmy.

        Also, mind checking out Deveraj's patch? I'd suggest at least reviewing it to figure if you fellas are using same conventions going all proto.

        Good stuff

        • Michael

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5511 ----------------------------------------------------------- pom.xml < https://reviews.apache.org/r/4054/#comment11930 > So, all proto generated files go to proto/generated? All into the same package? Thanks Jimmy. Also, mind checking out Deveraj's patch? I'd suggest at least reviewing it to figure if you fellas are using same conventions going all proto. Good stuff Michael On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-01 18:33:53, Michael Stack wrote:

        > pom.xml, line 746

        > <https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line746>

        >

        > Duh. Thanks Jimmy. What about the defines? Where are we going to generate the files into?

        This compile-proto.sh is under target.
        Those pb files are under target/generated-sources/java/org/apache/hadoop/hbase/proto/generated

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-01 18:33:53, Michael Stack wrote: > pom.xml, line 746 > < https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line746 > > > Duh. Thanks Jimmy. What about the defines? Where are we going to generate the files into? This compile-proto.sh is under target. Those pb files are under target/generated-sources/java/org/apache/hadoop/hbase/proto/generated Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5505 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        pom.xml
        <https://reviews.apache.org/r/4054/#comment11926>

        Duh. Thanks Jimmy. What about the defines? Where are we going to generate the files into?

        • Michael

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5505 ----------------------------------------------------------- pom.xml < https://reviews.apache.org/r/4054/#comment11926 > Duh. Thanks Jimmy. What about the defines? Where are we going to generate the files into? Michael On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-01 18:23:27, Michael Stack wrote:

        > pom.xml, line 746

        > <https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line746>

        >

        > I don't see this script in this patch

        This script is generated by the echo command here. The content of the script is inside the pom after this line.

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-01 18:23:27, Michael Stack wrote: > pom.xml, line 746 > < https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line746 > > > I don't see this script in this patch This script is generated by the echo command here. The content of the script is inside the pom after this line. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5503 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        pom.xml
        <https://reviews.apache.org/r/4054/#comment11924>

        I don't see this script in this patch

        • Michael

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5503 ----------------------------------------------------------- pom.xml < https://reviews.apache.org/r/4054/#comment11924 > I don't see this script in this patch Michael On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-28 22:59:39, Shaneal Manek wrote:

        > src/main/proto/HRegionProtocol.proto, line 84

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line84>

        >

        > What is this 'value' field? What is its serialization format?

        This value field is the value used to compare. It is a plain byte array.

        On 2012-02-28 22:59:39, Shaneal Manek wrote:

        > src/main/proto/HRegionProtocol.proto, line 28

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line28>

        >

        > Why is this called 'ColumnFamilyMapProto' (as opposed to just, say, 'ColumnProto' or 'ColumnNameProto')?

        It is a column family and a list of its columns. ColumnListProto, ColumnsProto?

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-28 22:59:39, Shaneal Manek wrote: > src/main/proto/HRegionProtocol.proto, line 84 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line84 > > > What is this 'value' field? What is its serialization format? This value field is the value used to compare. It is a plain byte array. On 2012-02-28 22:59:39, Shaneal Manek wrote: > src/main/proto/HRegionProtocol.proto, line 28 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line28 > > > Why is this called 'ColumnFamilyMapProto' (as opposed to just, say, 'ColumnProto' or 'ColumnNameProto')? It is a column family and a list of its columns. ColumnListProto, ColumnsProto? Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5415 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11804>

        Why is this called 'ColumnFamilyMapProto' (as opposed to just, say, 'ColumnProto' or 'ColumnNameProto')?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11805>

        What is this 'value' field? What is its serialization format?

        • Shaneal

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5415 ----------------------------------------------------------- src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11804 > Why is this called 'ColumnFamilyMapProto' (as opposed to just, say, 'ColumnProto' or 'ColumnNameProto')? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11805 > What is this 'value' field? What is its serialization format? Shaneal On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-28 23:08:15, Alex Newman wrote:

        > pom.xml, line 761

        > <https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line761>

        >

        > does it make sense to assume protoc is in path?

        Hadoop requires that. I think HBase can do the same thing too.

        On 2012-02-28 23:08:15, Alex Newman wrote:

        > src/main/proto/HRegionProtocol.proto, line 265

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line265>

        >

        > Why would you give a regionname, as opposed to a tablename?

        The current HRegionInterface uses regionName. It is to lock a row in a region. Otherwise, we need to find which region is the row in.

        On 2012-02-28 23:08:15, Alex Newman wrote:

        > src/main/proto/HRegionProtocol.proto, line 174

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line174>

        >

        > if the region is

        >

        > foo,bar,1234.4567.

        >

        > Then the encoded name is 4567? no?

        That's right. The encoded name is the hash.

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-28 23:08:15, Alex Newman wrote: > pom.xml, line 761 > < https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line761 > > > does it make sense to assume protoc is in path? Hadoop requires that. I think HBase can do the same thing too. On 2012-02-28 23:08:15, Alex Newman wrote: > src/main/proto/HRegionProtocol.proto, line 265 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line265 > > > Why would you give a regionname, as opposed to a tablename? The current HRegionInterface uses regionName. It is to lock a row in a region. Otherwise, we need to find which region is the row in. On 2012-02-28 23:08:15, Alex Newman wrote: > src/main/proto/HRegionProtocol.proto, line 174 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line174 > > > if the region is > > foo,bar,1234.4567. > > Then the encoded name is 4567? no? That's right. The encoded name is the hash. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5416 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > High-level, do we need to split the Interface more – into admin vs user protos? Would love to get rid of the plethora of methods but probably not a task to be done as part of this issue?

        Yes. Todd mentioned that too. Will do.

        On 2012-02-28 20:44:26, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line36>

        >

        > My guess is that there is no uint32?

        There is uint32. Should we use uint32 for timestamp? If it is in second, it is ok for many years. If it is in millisecond, we should use uint64.

        On 2012-02-28 20:44:26, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line42>

        >

        > Whats a 'name'? Is it a region name?

        It is for a generic NameValue pair.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > pom.xml, line 749

        > <https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line749>

        >

        > Do we do this elsewhere in the pom? If so, should set a boolean once?

        This is the only place.

        On 2012-02-28 20:44:26, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line1>

        >

        > So, all protobuf files go here? We need to ensure this the case w/ all patches (I think the rpc patch was putting them elsewhere.....)

        All proto files should go here, while Java files go to other places like other Java classes.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 21

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line21>

        >

        > Elsewhere in hbase the convention is 'what' and then the generated classes are in a 'generated' subpackage as in there is a thrift subpackage and under there are thrift utils and then a 'generated' subpackage. Ditto avro. This is different in that we have a generated top level subpackage w/ proto as a subpackage. Should we flip it?

        Sure, will do.

        On 2012-02-28 20:44:26, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line22>

        >

        > Should the suffix be Proto – our convention for Protobuf classes – or Protos? Why the plural? Perhaps this a container for a bunch of Proto?

        Yes, there are a bunch of messages. Each message is a Proto.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 23

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line23>

        >

        > This class seems to have more than HR stuff in it. Should we make more protos? A client proto?

        Ok.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 62

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line62>

        >

        > What is this? For filters?

        For checkAndPut, checkAndDelete, this is condition to check.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 88

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line88>

        >

        > Mutate is not deprecated? We don't have deprecated stuff in this proto file? This is doing what from current API?

        This Mutate is different from the mutateRow() in HRegionInterface. It is for append() and increment(). Any suggestion for a better naming?

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 105

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line105>

        >

        > Are these client datastructures? Or they go w/ the RS proto and are used by clients?

        For now, I prefer not to change the client data structures. So these go w/ the RS proto only and are not used by clients directly.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 118

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line118>

        >

        > Where does this come from in current API?

        Yes.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 148

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line148>

        >

        > WALEntryProto?

        Ok.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 126

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line126>

        >

        > Doesn't HRI have a tablename in it? So maybe this should have a HRI?

        >

        > What is this LogKeyProto modeling? Should be WALKeyProto?

        WAL log key. Will change the name.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 162

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line162>

        >

        > Should there be more in here?

        It should be bytes. I will it.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 165

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line165>

        >

        > Is this a class or method name? If method name is convention to capitalize?

        It is the request class.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 169

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line169>

        >

        > So, even if method returns a HRegionInfo, we return a GetRegionInfoResponseProto in case the response changes?

        That's right. The response proto contains the HRegionInfo in this case.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 174

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line174>

        >

        > regionName and encodedName and HRegionInfo class.... should we standardize?

        Yes, working on it.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 223

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line223>

        >

        > Maybe NextOnScannerRequestProto so relates better to our current API

        Sure. Will rename it.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 248

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line248>

        >

        > Man. Anyone use this thing?

        Could not find any reference. Should we deprecate it?

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 408

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line408>

        >

        > nextOnScanner?

        Will rename it.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 420

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line420>

        >

        > These are admin interface functions. Should we split the interface?

        Yes, working on it now.

        On 2012-02-28 20:44:26, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line62>

        >

        > Should this be uint32? (Maybe not possible in proto?)

        Is it better to use uint64? I assume it is in millisecond.

        On 2012-02-28 20:44:26, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line65>

        >

        > I think a bit of doc in here would not go amiss. Else its hard to figure all is going on in here; what goes where.

        >

        > Should ServerName be in this proto file?

        Yes, ServerName should be in this file.

        On 2012-02-28 20:44:26, Michael Stack wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line33>

        >

        > Doesn't an HRI have a Map?

        >

        > There does not seem to be enough in here.

        I did not in any map in HRegionInfo. Anything specific I missed?

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 464

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line464>

        >

        > Yeah, we need to doc. this later. Ain't the doc here going to be our Interface doc? Our only Interface doc?

        Right. Doc comes later.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 399

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line399>

        >

        > I wonder if these could take a more primitive type... a KV type.

        Then we may have problem to enhance it, for example, add/remove a parameter. In this case, we can always add/deprecate optional parameters to the Request.

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 315

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line315>

        >

        > We should have one or the other. Can have one call through to the other (thats implementation?)

        Will collapse closeRegionByName into closeRegion.

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-28 20:44:26, Michael Stack wrote: > High-level, do we need to split the Interface more – into admin vs user protos? Would love to get rid of the plethora of methods but probably not a task to be done as part of this issue? Yes. Todd mentioned that too. Will do. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/hbase.proto, line 36 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line36 > > > My guess is that there is no uint32? There is uint32. Should we use uint32 for timestamp? If it is in second, it is ok for many years. If it is in millisecond, we should use uint64. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/hbase.proto, line 42 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line42 > > > Whats a 'name'? Is it a region name? It is for a generic NameValue pair. On 2012-02-28 20:44:26, Michael Stack wrote: > pom.xml, line 749 > < https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line749 > > > Do we do this elsewhere in the pom? If so, should set a boolean once? This is the only place. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 1 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line1 > > > So, all protobuf files go here? We need to ensure this the case w/ all patches (I think the rpc patch was putting them elsewhere.....) All proto files should go here, while Java files go to other places like other Java classes. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 21 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line21 > > > Elsewhere in hbase the convention is 'what' and then the generated classes are in a 'generated' subpackage as in there is a thrift subpackage and under there are thrift utils and then a 'generated' subpackage. Ditto avro. This is different in that we have a generated top level subpackage w/ proto as a subpackage. Should we flip it? Sure, will do. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 22 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line22 > > > Should the suffix be Proto – our convention for Protobuf classes – or Protos? Why the plural? Perhaps this a container for a bunch of Proto? Yes, there are a bunch of messages. Each message is a Proto. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 23 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line23 > > > This class seems to have more than HR stuff in it. Should we make more protos? A client proto? Ok. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 62 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line62 > > > What is this? For filters? For checkAndPut, checkAndDelete, this is condition to check. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 88 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line88 > > > Mutate is not deprecated? We don't have deprecated stuff in this proto file? This is doing what from current API? This Mutate is different from the mutateRow() in HRegionInterface. It is for append() and increment(). Any suggestion for a better naming? On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 105 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line105 > > > Are these client datastructures? Or they go w/ the RS proto and are used by clients? For now, I prefer not to change the client data structures. So these go w/ the RS proto only and are not used by clients directly. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 118 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line118 > > > Where does this come from in current API? Yes. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 148 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line148 > > > WALEntryProto? Ok. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 126 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line126 > > > Doesn't HRI have a tablename in it? So maybe this should have a HRI? > > What is this LogKeyProto modeling? Should be WALKeyProto? WAL log key. Will change the name. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 162 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line162 > > > Should there be more in here? It should be bytes. I will it. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 165 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line165 > > > Is this a class or method name? If method name is convention to capitalize? It is the request class. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 169 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line169 > > > So, even if method returns a HRegionInfo, we return a GetRegionInfoResponseProto in case the response changes? That's right. The response proto contains the HRegionInfo in this case. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 174 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line174 > > > regionName and encodedName and HRegionInfo class.... should we standardize? Yes, working on it. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 223 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line223 > > > Maybe NextOnScannerRequestProto so relates better to our current API Sure. Will rename it. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 248 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line248 > > > Man. Anyone use this thing? Could not find any reference. Should we deprecate it? On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 408 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line408 > > > nextOnScanner? Will rename it. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 420 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line420 > > > These are admin interface functions. Should we split the interface? Yes, working on it now. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/hbase.proto, line 62 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line62 > > > Should this be uint32? (Maybe not possible in proto?) Is it better to use uint64? I assume it is in millisecond. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/hbase.proto, line 65 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line65 > > > I think a bit of doc in here would not go amiss. Else its hard to figure all is going on in here; what goes where. > > Should ServerName be in this proto file? Yes, ServerName should be in this file. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/hbase.proto, line 33 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line33 > > > Doesn't an HRI have a Map? > > There does not seem to be enough in here. I did not in any map in HRegionInfo. Anything specific I missed? On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 464 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line464 > > > Yeah, we need to doc. this later. Ain't the doc here going to be our Interface doc? Our only Interface doc? Right. Doc comes later. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 399 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line399 > > > I wonder if these could take a more primitive type... a KV type. Then we may have problem to enhance it, for example, add/remove a parameter. In this case, we can always add/deprecate optional parameters to the Request. On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 315 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line315 > > > We should have one or the other. Can have one call through to the other (thats implementation?) Will collapse closeRegionByName into closeRegion. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5407 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        pom.xml
        <https://reviews.apache.org/r/4054/#comment11821>

        It's not too bad - a bunch of hadoop-common stuff already does. (e.g. https://github.com/apache/hadoop-common/blob/branch-0.23/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/pom.xml)

        • Shaneal

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5421 ----------------------------------------------------------- pom.xml < https://reviews.apache.org/r/4054/#comment11821 > It's not too bad - a bunch of hadoop-common stuff already does. (e.g. https://github.com/apache/hadoop-common/blob/branch-0.23/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/pom.xml ) Shaneal On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-28 20:44:26, Michael Stack wrote:

        > src/main/proto/HRegionProtocol.proto, line 118

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line118>

        >

        > Where does this come from in current API?

        Jimmy Xiang wrote:

        Yes.

        It is from the current API. It seems nobody is using it. I will get rid of it.

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-28 20:44:26, Michael Stack wrote: > src/main/proto/HRegionProtocol.proto, line 118 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line118 > > > Where does this come from in current API? Jimmy Xiang wrote: Yes. It is from the current API. It seems nobody is using it. I will get rid of it. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5407 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        pom.xml
        <https://reviews.apache.org/r/4054/#comment11806>

        does it make sense to assume protoc is in path?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11807>

        HRI does have the tablename. Correct.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11808>

        if the region is

        foo,bar,1234.4567.

        Then the encoded name is 4567? no?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11809>

        Why would you give a regionname, as opposed to a tablename?

        • Alex

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5416 ----------------------------------------------------------- pom.xml < https://reviews.apache.org/r/4054/#comment11806 > does it make sense to assume protoc is in path? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11807 > HRI does have the tablename. Correct. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11808 > if the region is foo,bar,1234.4567. Then the encoded name is 4567? no? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11809 > Why would you give a regionname, as opposed to a tablename? Alex On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-28 23:28:19, Shaneal Manek wrote:

        > pom.xml, line 761

        > <https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line761>

        >

        > It's not too bad - a bunch of hadoop-common stuff already does. (e.g. https://github.com/apache/hadoop-common/blob/branch-0.23/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/pom.xml)

        You are right. I copied over from there.

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-28 23:28:19, Shaneal Manek wrote: > pom.xml, line 761 > < https://reviews.apache.org/r/4054/diff/1/?file=86002#file86002line761 > > > It's not too bad - a bunch of hadoop-common stuff already does. (e.g. https://github.com/apache/hadoop-common/blob/branch-0.23/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/pom.xml ) You are right. I copied over from there. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5421 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        High-level, do we need to split the Interface more – into admin vs user protos? Would love to get rid of the plethora of methods but probably not a task to be done as part of this issue?

        pom.xml
        <https://reviews.apache.org/r/4054/#comment11742>

        There are prereqs for this to work? I suppose compile-proto.sh checks and its failing it seems fails the build.. thats good.

        pom.xml
        <https://reviews.apache.org/r/4054/#comment11741>

        Do we do this elsewhere in the pom? If so, should set a boolean once?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11743>

        So, all protobuf files go here? We need to ensure this the case w/ all patches (I think the rpc patch was putting them elsewhere.....)

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11744>

        Elsewhere in hbase the convention is 'what' and then the generated classes are in a 'generated' subpackage as in there is a thrift subpackage and under there are thrift utils and then a 'generated' subpackage. Ditto avro. This is different in that we have a generated top level subpackage w/ proto as a subpackage. Should we flip it?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11745>

        Should the suffix be Proto – our convention for Protobuf classes – or Protos? Why the plural? Perhaps this a container for a bunch of Proto?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11755>

        This class seems to have more than HR stuff in it. Should we make more protos? A client proto?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11751>

        Mutation (Probably already used)

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11752>

        What is this? For filters?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11753>

        Mutate is not deprecated? We don't have deprecated stuff in this proto file? This is doing what from current API?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11756>

        Are these client datastructures? Or they go w/ the RS proto and are used by clients?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11754>

        Where does this come from in current API?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11758>

        Doesn't HRI have a tablename in it? So maybe this should have a HRI?

        What is this LogKeyProto modeling? Should be WALKeyProto?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11759>

        WALEntryProto?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11760>

        Should there be more in here?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11761>

        Is this a class or method name? If method name is convention to capitalize?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11762>

        So, even if method returns a HRegionInfo, we return a GetRegionInfoResponseProto in case the response changes?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11763>

        regionName and encodedName and HRegionInfo class.... should we standardize?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11764>

        Excellent. Later we need to return first set of results in here rather than have to have client go back to do a next.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11765>

        Maybe NextOnScannerRequestProto so relates better to our current API

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11766>

        We should make this optional some day.. that you have to call a close.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11767>

        Man. Anyone use this thing?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11768>

        We should have one or the other. Can have one call through to the other (thats implementation?)

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11769>

        Oh, I see how method naming and classes goes

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11770>

        I wonder if these could take a more primitive type... a KV type.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11771>

        nextOnScanner?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11772>

        These are admin interface functions. Should we split the interface?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11773>

        Yeah, we need to doc. this later. Ain't the doc here going to be our Interface doc? Our only Interface doc?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment11750>

        Doesn't an HRI have a Map?

        There does not seem to be enough in here.

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment11748>

        My guess is that there is no uint32?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment11746>

        Whats a 'name'? Is it a region name?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment11747>

        Should this be uint32? (Maybe not possible in proto?)

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment11749>

        I think a bit of doc in here would not go amiss. Else its hard to figure all is going on in here; what goes where.

        Should ServerName be in this proto file?

        • Michael

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5407 ----------------------------------------------------------- High-level, do we need to split the Interface more – into admin vs user protos? Would love to get rid of the plethora of methods but probably not a task to be done as part of this issue? pom.xml < https://reviews.apache.org/r/4054/#comment11742 > There are prereqs for this to work? I suppose compile-proto.sh checks and its failing it seems fails the build.. thats good. pom.xml < https://reviews.apache.org/r/4054/#comment11741 > Do we do this elsewhere in the pom? If so, should set a boolean once? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11743 > So, all protobuf files go here? We need to ensure this the case w/ all patches (I think the rpc patch was putting them elsewhere.....) src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11744 > Elsewhere in hbase the convention is 'what' and then the generated classes are in a 'generated' subpackage as in there is a thrift subpackage and under there are thrift utils and then a 'generated' subpackage. Ditto avro. This is different in that we have a generated top level subpackage w/ proto as a subpackage. Should we flip it? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11745 > Should the suffix be Proto – our convention for Protobuf classes – or Protos? Why the plural? Perhaps this a container for a bunch of Proto? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11755 > This class seems to have more than HR stuff in it. Should we make more protos? A client proto? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11751 > Mutation (Probably already used) src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11752 > What is this? For filters? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11753 > Mutate is not deprecated? We don't have deprecated stuff in this proto file? This is doing what from current API? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11756 > Are these client datastructures? Or they go w/ the RS proto and are used by clients? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11754 > Where does this come from in current API? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11758 > Doesn't HRI have a tablename in it? So maybe this should have a HRI? What is this LogKeyProto modeling? Should be WALKeyProto? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11759 > WALEntryProto? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11760 > Should there be more in here? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11761 > Is this a class or method name? If method name is convention to capitalize? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11762 > So, even if method returns a HRegionInfo, we return a GetRegionInfoResponseProto in case the response changes? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11763 > regionName and encodedName and HRegionInfo class.... should we standardize? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11764 > Excellent. Later we need to return first set of results in here rather than have to have client go back to do a next. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11765 > Maybe NextOnScannerRequestProto so relates better to our current API src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11766 > We should make this optional some day.. that you have to call a close. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11767 > Man. Anyone use this thing? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11768 > We should have one or the other. Can have one call through to the other (thats implementation?) src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11769 > Oh, I see how method naming and classes goes src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11770 > I wonder if these could take a more primitive type... a KV type. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11771 > nextOnScanner? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11772 > These are admin interface functions. Should we split the interface? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11773 > Yeah, we need to doc. this later. Ain't the doc here going to be our Interface doc? Our only Interface doc? src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment11750 > Doesn't an HRI have a Map? There does not seem to be enough in here. src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment11748 > My guess is that there is no uint32? src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment11746 > Whats a 'name'? Is it a region name? src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment11747 > Should this be uint32? (Maybe not possible in proto?) src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment11749 > I think a bit of doc in here would not go amiss. Else its hard to figure all is going on in here; what goes where. Should ServerName be in this proto file? Michael On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-27 23:52:25, Todd Lipcon wrote:

        > src/main/proto/HRegionProtocol.proto, line 323

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line323>

        >

        > oh, right... here's another place we could use HRegionSpecifier or something, then?

        compactRegion/closeRegion/openRegion/splitRegion/flushRegion all can use it, right?

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-27 23:52:25, Todd Lipcon wrote: > src/main/proto/HRegionProtocol.proto, line 323 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line323 > > > oh, right... here's another place we could use HRegionSpecifier or something, then? compactRegion/closeRegion/openRegion/splitRegion/flushRegion all can use it, right? Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5372 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11701>

        oh, right... here's another place we could use HRegionSpecifier or something, then?

        • Todd

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5372 ----------------------------------------------------------- src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11701 > oh, right... here's another place we could use HRegionSpecifier or something, then? Todd On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-27 22:54:55, Gregory Chanan wrote:

        > I notice you followed the same declaration order as HRegionInterface. Could we reorder these along say, functionality lines with appropriate comments in the .proto file?

        > NOTE: I see Todd suggested splitting along management/client lines. That may be superior. Here's a sketch of what I think "along functionality lines" would look like:

        >

        > // Row(s) level

        > get

        > put

        > delete

        > mutate

        > openScanner

        > fetchFromScanner

        > closeScanner

        > lockRow

        > unlockRow

        >

        > // Region level info

        > getRegionInfo

        > getOnlineRegions

        > getLastFlushTime

        > getStoreFileList

        >

        > // Region-level mutation

        > flushRegion

        > openRegion

        > closeRegion

        > closeRegionByEncodedName

        > splitRegion

        > compactRegion

        > bulkLoadHFiles

        > execCoprocessor

        >

        > // RegionServer-level

        > getBlockCacheColumnFamilySummaries

        > replicateLogEntry

        > rollLogWriter

        > stopServer

        >

        Good idea. Will do.

        On 2012-02-27 22:54:55, Gregory Chanan wrote:

        > src/main/proto/HRegionProtocol.proto, line 88

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line88>

        >

        > The name mutate doesn't seem consistent to me. For one, "mutate" seems to not support Put/Deletes, but RowMutation only seems to support Puts/Deletes. Perhaps we can collapse this somehow?

        Yes. I think so. I used to collapse both put and delete to one call putOrDelete. But it sounds not very good to me. If we collapse them, what do you want it to be called?

        On 2012-02-27 22:54:55, Gregory Chanan wrote:

        > src/main/proto/HRegionProtocol.proto, line 121

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line121>

        >

        > this should be heapSize.

        Will fix.

        On 2012-02-27 22:54:55, Gregory Chanan wrote:

        > src/main/proto/HRegionProtocol.proto, line 402

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line402>

        >

        > You got rid of mutateRow and suggest the new put/delete can support as along as there are no mixed puts and deletes.

        >

        > But it looks like mutateRow allowed you to do mixed puts and deletes atomically. Could you support atomic puts and deletes with what you have here?

        If we collapse put and delete, it should be supported.

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-27 22:54:55, Gregory Chanan wrote: > I notice you followed the same declaration order as HRegionInterface. Could we reorder these along say, functionality lines with appropriate comments in the .proto file? > NOTE: I see Todd suggested splitting along management/client lines. That may be superior. Here's a sketch of what I think "along functionality lines" would look like: > > // Row(s) level > get > put > delete > mutate > openScanner > fetchFromScanner > closeScanner > lockRow > unlockRow > > // Region level info > getRegionInfo > getOnlineRegions > getLastFlushTime > getStoreFileList > > // Region-level mutation > flushRegion > openRegion > closeRegion > closeRegionByEncodedName > splitRegion > compactRegion > bulkLoadHFiles > execCoprocessor > > // RegionServer-level > getBlockCacheColumnFamilySummaries > replicateLogEntry > rollLogWriter > stopServer > Good idea. Will do. On 2012-02-27 22:54:55, Gregory Chanan wrote: > src/main/proto/HRegionProtocol.proto, line 88 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line88 > > > The name mutate doesn't seem consistent to me. For one, "mutate" seems to not support Put/Deletes, but RowMutation only seems to support Puts/Deletes. Perhaps we can collapse this somehow? Yes. I think so. I used to collapse both put and delete to one call putOrDelete. But it sounds not very good to me. If we collapse them, what do you want it to be called? On 2012-02-27 22:54:55, Gregory Chanan wrote: > src/main/proto/HRegionProtocol.proto, line 121 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line121 > > > this should be heapSize. Will fix. On 2012-02-27 22:54:55, Gregory Chanan wrote: > src/main/proto/HRegionProtocol.proto, line 402 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line402 > > > You got rid of mutateRow and suggest the new put/delete can support as along as there are no mixed puts and deletes. > > But it looks like mutateRow allowed you to do mixed puts and deletes atomically. Could you support atomic puts and deletes with what you have here? If we collapse put and delete, it should be supported. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5363 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-27 22:31:45, Todd Lipcon wrote:

        > here are some initial thoughts, let's make sure we get good discussion on this before commit.

        Of course. This is just the first draft.

        On 2012-02-27 22:31:45, Todd Lipcon wrote:

        > src/main/proto/HRegionProtocol.proto, lines 48-60

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line48>

        >

        > I think we could collapse Put and Delete

        I'd like to. That means we can collapse both call put() and delete() too. If so, what's a good name?

        On 2012-02-27 22:31:45, Todd Lipcon wrote:

        > src/main/proto/HRegionProtocol.proto, line 83

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line83>

        >

        > typo

        Will fix.

        On 2012-02-27 22:31:45, Todd Lipcon wrote:

        > src/main/proto/HRegionProtocol.proto, lines 88-102

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line88>

        >

        > we seem to be missing a row here.

        Good catch. Will add it.

        On 2012-02-27 22:31:45, Todd Lipcon wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line27>

        >

        > isn't the regionName just the concatenation of other fields here?

        Will remove it.

        On 2012-02-27 22:31:45, Todd Lipcon wrote:

        > src/main/proto/HRegionProtocol.proto, line 323

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line323>

        >

        > why do we need a regioninfo here? we can locate the region by its split point, can't we?

        split point is an optional row. I think we do need regioninfo.

        On 2012-02-27 22:31:45, Todd Lipcon wrote:

        > src/main/proto/hbase.proto, lines 31-32

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line31>

        >

        > these two don't seem to belong here, since they're transient state rather than properties of the region itself

        Will move them somewhere else.

        On 2012-02-27 22:31:45, Todd Lipcon wrote:

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

        > <https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line38>

        >

        > when do you set allTime? isn't "allTime" the same as setting both of the above to null?

        Right. Will remove it.

        On 2012-02-27 22:31:45, Todd Lipcon wrote:

        > src/main/proto/HRegionProtocol.proto, line 313

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line313>

        >

        > Can this be collapsed with CloseRegion?

        > Maybe we can have a proto like:

        > message RegionSpecifierProto {

        > required RegionSpecifierType type = 1;

        > required bytes data = 2;

        > enum RegionSpecifierType { bq. > BY_REGION_ID_SHA1=0, bq. > BY_CONTAINED_ROW=1, bq. > BY_FULL_NAME=2 bq. > }

        > }

        > or something? Seems like it would be useful throughout to refer to regions in places where it's nice to have flexibility

        I can collapse it with closeRegion by making both regionInfo and encodedRegionName optional.

        On 2012-02-27 22:31:45, Todd Lipcon wrote:

        > src/main/proto/HRegionProtocol.proto, line 124

        > <https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line124>

        >

        > Why are the HLog-related things under HRegionProtocol? Seems like these shouldn't be exposed to clients.

        For management and replication related things, should I move them to a separate interface, called HRegionMasterInterface, HRegionManagementInterface, HManagementInterface, or something else?
        We already have a HMasterRegionInterface.

        • Jimmy

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

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-27 22:31:45, Todd Lipcon wrote: > here are some initial thoughts, let's make sure we get good discussion on this before commit. Of course. This is just the first draft. On 2012-02-27 22:31:45, Todd Lipcon wrote: > src/main/proto/HRegionProtocol.proto, lines 48-60 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line48 > > > I think we could collapse Put and Delete I'd like to. That means we can collapse both call put() and delete() too. If so, what's a good name? On 2012-02-27 22:31:45, Todd Lipcon wrote: > src/main/proto/HRegionProtocol.proto, line 83 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line83 > > > typo Will fix. On 2012-02-27 22:31:45, Todd Lipcon wrote: > src/main/proto/HRegionProtocol.proto, lines 88-102 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line88 > > > we seem to be missing a row here. Good catch. Will add it. On 2012-02-27 22:31:45, Todd Lipcon wrote: > src/main/proto/hbase.proto, line 27 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line27 > > > isn't the regionName just the concatenation of other fields here? Will remove it. On 2012-02-27 22:31:45, Todd Lipcon wrote: > src/main/proto/HRegionProtocol.proto, line 323 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line323 > > > why do we need a regioninfo here? we can locate the region by its split point, can't we? split point is an optional row. I think we do need regioninfo. On 2012-02-27 22:31:45, Todd Lipcon wrote: > src/main/proto/hbase.proto, lines 31-32 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line31 > > > these two don't seem to belong here, since they're transient state rather than properties of the region itself Will move them somewhere else. On 2012-02-27 22:31:45, Todd Lipcon wrote: > src/main/proto/hbase.proto, line 38 > < https://reviews.apache.org/r/4054/diff/1/?file=86004#file86004line38 > > > when do you set allTime? isn't "allTime" the same as setting both of the above to null? Right. Will remove it. On 2012-02-27 22:31:45, Todd Lipcon wrote: > src/main/proto/HRegionProtocol.proto, line 313 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line313 > > > Can this be collapsed with CloseRegion? > Maybe we can have a proto like: > message RegionSpecifierProto { > required RegionSpecifierType type = 1; > required bytes data = 2; > enum RegionSpecifierType { bq. > BY_REGION_ID_SHA1=0, bq. > BY_CONTAINED_ROW=1, bq. > BY_FULL_NAME=2 bq. > } > } > or something? Seems like it would be useful throughout to refer to regions in places where it's nice to have flexibility I can collapse it with closeRegion by making both regionInfo and encodedRegionName optional. On 2012-02-27 22:31:45, Todd Lipcon wrote: > src/main/proto/HRegionProtocol.proto, line 124 > < https://reviews.apache.org/r/4054/diff/1/?file=86003#file86003line124 > > > Why are the HLog-related things under HRegionProtocol? Seems like these shouldn't be exposed to clients. For management and replication related things, should I move them to a separate interface, called HRegionMasterInterface, HRegionManagementInterface, HManagementInterface, or something else? We already have a HMasterRegionInterface. Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5364 ----------------------------------------------------------- On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        I notice you followed the same declaration order as HRegionInterface. Could we reorder these along say, functionality lines with appropriate comments in the .proto file?
        NOTE: I see Todd suggested splitting along management/client lines. That may be superior. Here's a sketch of what I think "along functionality lines" would look like:

        // Row(s) level
        get
        put
        delete
        mutate
        openScanner
        fetchFromScanner
        closeScanner
        lockRow
        unlockRow

        // Region level info
        getRegionInfo
        getOnlineRegions
        getLastFlushTime
        getStoreFileList

        // Region-level mutation
        flushRegion
        openRegion
        closeRegion
        closeRegionByEncodedName
        splitRegion
        compactRegion
        bulkLoadHFiles
        execCoprocessor

        // RegionServer-level
        getBlockCacheColumnFamilySummaries
        replicateLogEntry
        rollLogWriter
        stopServer

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11669>

        First enum is missing a comment. I might just get rid of the comments though, I don't think they really add anything.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11687>

        The name mutate doesn't seem consistent to me. For one, "mutate" seems to not support Put/Deletes, but RowMutation only seems to support Puts/Deletes. Perhaps we can collapse this somehow?

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11670>

        this should be heapSize.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11688>

        You got rid of mutateRow and suggest the new put/delete can support as along as there are no mixed puts and deletes.

        But it looks like mutateRow allowed you to do mixed puts and deletes atomically. Could you support atomic puts and deletes with what you have here?

        • Gregory

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5363 ----------------------------------------------------------- I notice you followed the same declaration order as HRegionInterface. Could we reorder these along say, functionality lines with appropriate comments in the .proto file? NOTE: I see Todd suggested splitting along management/client lines. That may be superior. Here's a sketch of what I think "along functionality lines" would look like: // Row(s) level get put delete mutate openScanner fetchFromScanner closeScanner lockRow unlockRow // Region level info getRegionInfo getOnlineRegions getLastFlushTime getStoreFileList // Region-level mutation flushRegion openRegion closeRegion closeRegionByEncodedName splitRegion compactRegion bulkLoadHFiles execCoprocessor // RegionServer-level getBlockCacheColumnFamilySummaries replicateLogEntry rollLogWriter stopServer src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11669 > First enum is missing a comment. I might just get rid of the comments though, I don't think they really add anything. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11687 > The name mutate doesn't seem consistent to me. For one, "mutate" seems to not support Put/Deletes, but RowMutation only seems to support Puts/Deletes. Perhaps we can collapse this somehow? src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11670 > this should be heapSize. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11688 > You got rid of mutateRow and suggest the new put/delete can support as along as there are no mixed puts and deletes. But it looks like mutateRow allowed you to do mixed puts and deletes atomically. Could you support atomic puts and deletes with what you have here? Gregory On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        here are some initial thoughts, let's make sure we get good discussion on this before commit.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11671>

        I think we could collapse Put and Delete

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11672>

        typo

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11673>

        we seem to be missing a row here.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11674>

        Why are the HLog-related things under HRegionProtocol? Seems like these shouldn't be exposed to clients.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11675>

        would be nice to extract these "management interface" things from this protocol – so the stuff that the average client needs goes in one place, and the stuff that only management tools would use go in another.

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11676>

        Also these calls which are supposed to only be called by the master, maybe we can move elsewhere

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11677>

        Can this be collapsed with CloseRegion?
        Maybe we can have a proto like:
        message RegionSpecifierProto {
        required RegionSpecifierType type = 1;
        required bytes data = 2;
        enum RegionSpecifierType

        { BY_REGION_ID_SHA1=0, BY_CONTAINED_ROW=1, BY_FULL_NAME=2 }

        }
        or something? Seems like it would be useful throughout to refer to regions in places where it's nice to have flexibility

        src/main/proto/HRegionProtocol.proto
        <https://reviews.apache.org/r/4054/#comment11678>

        why do we need a regioninfo here? we can locate the region by its split point, can't we?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment11680>

        isn't the regionName just the concatenation of other fields here?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment11679>

        these two don't seem to belong here, since they're transient state rather than properties of the region itself

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4054/#comment11681>

        when do you set allTime? isn't "allTime" the same as setting both of the above to null?

        • Todd

        On 2012-02-27 18:54:31, Jimmy Xiang wrote:

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

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

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

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

        (Updated 2012-02-27 18:54:31)

        Review request for hbase.

        Summary

        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

        This addresses bug HBASE-5443.

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

        Diffs

        -----

        pom.xml 066c027

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

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

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

        Testing

        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/#review5364 ----------------------------------------------------------- here are some initial thoughts, let's make sure we get good discussion on this before commit. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11671 > I think we could collapse Put and Delete src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11672 > typo src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11673 > we seem to be missing a row here. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11674 > Why are the HLog-related things under HRegionProtocol? Seems like these shouldn't be exposed to clients. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11675 > would be nice to extract these "management interface" things from this protocol – so the stuff that the average client needs goes in one place, and the stuff that only management tools would use go in another. src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11676 > Also these calls which are supposed to only be called by the master, maybe we can move elsewhere src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11677 > Can this be collapsed with CloseRegion? Maybe we can have a proto like: message RegionSpecifierProto { required RegionSpecifierType type = 1; required bytes data = 2; enum RegionSpecifierType { BY_REGION_ID_SHA1=0, BY_CONTAINED_ROW=1, BY_FULL_NAME=2 } } or something? Seems like it would be useful throughout to refer to regions in places where it's nice to have flexibility src/main/proto/HRegionProtocol.proto < https://reviews.apache.org/r/4054/#comment11678 > why do we need a regioninfo here? we can locate the region by its split point, can't we? src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment11680 > isn't the regionName just the concatenation of other fields here? src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment11679 > these two don't seem to belong here, since they're transient state rather than properties of the region itself src/main/proto/hbase.proto < https://reviews.apache.org/r/4054/#comment11681 > when do you set allTime? isn't "allTime" the same as setting both of the above to null? Todd On 2012-02-27 18:54:31, Jimmy Xiang wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- (Updated 2012-02-27 18:54:31) Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs ----- pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        Ted Yu added a comment -

        That would break one RPC call into multiple RPC calls, right ?

        Show
        Ted Yu added a comment - That would break one RPC call into multiple RPC calls, right ?
        Hide
        Jimmy Xiang added a comment -

        We can still support multi(MultiAction). Should we still support it in the RPC layer?
        Can we put some logic in the client side, like aggregating the actions based
        on region, action type (put/delete/get), and so on?

        Show
        Jimmy Xiang added a comment - We can still support multi(MultiAction). Should we still support it in the RPC layer? Can we put some logic in the client side, like aggregating the actions based on region, action type (put/delete/get), and so on?
        Hide
        Ted Yu added a comment -

        I think we should keep supporting multi(MultiAction) - asynchbase uses it heavily.
        Take a look at src/MultiAction.java in asynchbase

        Show
        Ted Yu added a comment - I think we should keep supporting multi(MultiAction) - asynchbase uses it heavily. Take a look at src/MultiAction.java in asynchbase
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Review request for hbase.

        Summary
        -------

        This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443

        Please review. I'd like to move ahead after we get to some agreement.

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

        Diffs


        pom.xml 066c027
        src/main/proto/HRegionProtocol.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION

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

        Testing
        -------

        Thanks,

        Jimmy

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4054/ ----------------------------------------------------------- Review request for hbase. Summary ------- This is the first draft of the ProtoBuff HRegionProtocol. The corresponding java vs pb method mapping is attached to the jira: https://issues.apache.org/jira/browse/HBASE-5443 Please review. I'd like to move ahead after we get to some agreement. This addresses bug HBASE-5443 . https://issues.apache.org/jira/browse/HBASE-5443 Diffs pom.xml 066c027 src/main/proto/HRegionProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4054/diff Testing ------- Thanks, Jimmy
        Hide
        Jimmy Xiang added a comment -

        In the attached region_java-proto-mapping.pdf file, I have the first draft of proto rpc methods to the java interface mapping.

        Please review, especially the following questions:

        1. Should we combine put and delete to one method? Their signatures are kind of the same.

        2. mutateRow(regionName, RowMutations)

        For multiple puts or deletes, the new put/delete can support as long as there are no mixed puts and deletes.
        Do we have to support mixed puts and deletes in RPC? Can we separate them in HBaseClient?

        3. multi(MultiAction)

        This one is too complicated. I think we should not put this in RPC. We can separate them in HBaseClient
        and call corresponding puts/deletes/gets?

        Show
        Jimmy Xiang added a comment - In the attached region_java-proto-mapping.pdf file, I have the first draft of proto rpc methods to the java interface mapping. Please review, especially the following questions: 1. Should we combine put and delete to one method? Their signatures are kind of the same. 2. mutateRow(regionName, RowMutations) For multiple puts or deletes, the new put/delete can support as long as there are no mixed puts and deletes. Do we have to support mixed puts and deletes in RPC? Can we separate them in HBaseClient? 3. multi(MultiAction) This one is too complicated. I think we should not put this in RPC. We can separate them in HBaseClient and call corresponding puts/deletes/gets?

          People

          • Assignee:
            Jimmy Xiang
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development