[ https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252617#comment-13252617 ]
jirapos...@reviews.apache.org commented on HBASE-5620: ------------------------------------------------------ bq. On 2012-04-12 04:50:15, Michael Stack wrote: bq. > What you want to do Jimmy? On next iteration, if all tests pass, should we check it in? I think its missing unit tests for the new protobuf utility. We should add those. I'd be fine adding them in a separate patch if that would suit you. This thing is already massive. We could commit to a branch too but that probably doesn't get us far; you have your own setup where you are to which you can commit, right? Could you please check it in if all tests pass? It can save me some rebasing/merging efforts. :) I can add unit tests for the new protobuf utility in a separate patch. I do have my own setup to commit my changes. I am also working on HBASE-5621. I can resolve other issues in HBASE-5621, or separate patches. bq. On 2012-04-12 04:50:15, Michael Stack wrote: bq. > src/main/protobuf/Client.proto, line 56 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100900#file100900line56> bq. > bq. > Call this keyValueBytes then so method names give more of a clue as to what you'll be getting; save on the surprises? Sure, will fix. bq. On 2012-04-12 04:50:15, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, line 143 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100898#file100898line143> bq. > bq. > Should this class implement Closeable, Stoppable and Abortable? RegionServerServices defined Stoppable, Abortable. Now RegionServer implements RegionServerServices. Not sure about Closeable. bq. On 2012-04-12 04:50:15, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, line 98 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100898#file100898line98> bq. > bq. > Shouldn't this implement Server and RegionServerServices? bq. > bq. > I would love to take the time to break this class up into smaller pieces but thats not for this JIRA. Can do that after this goes in. RegionServer now implements RegionServerServices which extends Server. It will be great to break this class up. bq. On 2012-04-12 04:50:15, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, line 94 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100898#file100898line94> bq. > bq. > Fix comment. bq. > bq. > Should you explain your intent for this class too? That HRegionServer will wither away? I think it woud be useful since it was not plain to me at first but after you explained it, I was on your side. Sure, added comments. Thanks. - Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4629/#review6868 ----------------------------------------------------------- On 2012-04-07 21:30:32, Jimmy Xiang wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/4629/ bq. ----------------------------------------------------------- bq. bq. (Updated 2012-04-07 21:30:32) bq. bq. bq. Review request for hbase. bq. bq. bq. Summary bq. ------- bq. bq. This is the client protocol part of region interface. The admin protocol part will be done in a different jira. bq. bq. The HRegionInterface is still there since the admin part is not done yet. The other reason is that in case some people still wants the old interface bq. bq. Filters, comparators and coprocessor parameters are still Writable. They will be addressed in different jiras. bq. bq. The existing client interface is not changed so that we don't break existing clients. bq. bq. bq. This addresses bug HBASE-5620. bq. https://issues.apache.org/jira/browse/HBASE-5620 bq. bq. bq. Diffs bq. ----- bq. bq. src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java 0129ee9 bq. src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 97293aa bq. src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 16e4017 bq. src/main/java/org/apache/hadoop/hbase/client/HConnection.java 5d43086 bq. src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 0b783ce bq. src/main/java/org/apache/hadoop/hbase/client/HTable.java aa7652f bq. src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java 9903df3 bq. src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java ddcf9ad bq. src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java 1acbdab bq. src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 bq. src/main/java/org/apache/hadoop/hbase/io/TimeRange.java d135393 bq. src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 05ae717 bq. src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 bq. src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 1316457 bq. src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java 19ae18c bq. src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 2eb57de bq. src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java 4026da0 bq. src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionAdminProtos.java 2169310 bq. src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java b36a9c0 bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8a61f7d bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 703e73d bq. src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java b520f3f bq. src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java PRE-CREATION bq. src/main/protobuf/Admin.proto PRE-CREATION bq. src/main/protobuf/Client.proto PRE-CREATION bq. src/main/protobuf/RegionAdmin.proto c64d68b bq. src/main/protobuf/RegionClient.proto 358382b bq. src/main/protobuf/hbase.proto da78788 bq. src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java c7284dc bq. src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java 0042468 bq. src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java e34d8bc bq. src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 bq. src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java 41616c8 bq. src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 6ed4ba2 bq. src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java b4dcb83 bq. src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java ceca6f5 bq. src/test/java/org/apache/hadoop/hbase/regionserver/OOMERegionServer.java cac2989 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java a1bf73b bq. bq. Diff: https://reviews.apache.org/r/4629/diff bq. bq. bq. Testing bq. ------- bq. bq. "mvn -PrunAllTests clean test" is green, except some flaky tests which need to run again. bq. bq. Also tested it on a real cluster with ycsb and bigtop. bq. bq. bq. Thanks, bq. bq. Jimmy bq. bq. > Convert the client protocol of HRegionInterface to PB > ----------------------------------------------------- > > Key: HBASE-5620 > URL: https://issues.apache.org/jira/browse/HBASE-5620 > Project: HBase > Issue Type: Sub-task > Components: ipc, master, migration, regionserver > Reporter: Jimmy Xiang > Assignee: Jimmy Xiang > Fix For: 0.96.0 > > -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira