[ https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13251719#comment-13251719 ]
jirapos...@reviews.apache.org commented on HBASE-5620: ------------------------------------------------------ bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > I got as far as HTable. Still have more to go. This is some pretty great work Jimmy. This stuff is hard. One thought I was having that is a little related (but it can be safely ignored) is what if there were only one method on an HRegionServer and you gave it an array of Gets, Puts, Delete, Increment, pb messages and you just let the regionserver sort it out. is that even posssible? I'll do rest of review tomorrow. Thanks for reviewing. As to one method to handle all actions, it is supported by the multi call. It is too complex to match the response. bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java, line 691 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100870#file100870line691> bq. > bq. > I said it before but I like this code removal It is not used so I removed it. bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 533 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100872#file100872line533> bq. > bq. > What about the EOFE you added today? Does that need to go in here anywhere? EOFE is an IOException. Good catch. I addressed it in HBASE-5621, which is in testing now. bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 574 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100872#file100872line574> bq. > bq. > So, to be clear, if a client does not close a scanner now, how does it get cleaned up on the server? Does it live forever? In this case, the Leases daemon will expire the scanner and clean it up. bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 184 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100874#file100874line184> bq. > bq. > Rename to hbase.clientprotocol.class and CLIENT_PROTOCOL_CLASS. Seems more consistent? Will fix. bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 235 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100873#file100873line235> bq. > bq. > Does this feel right Jimmy? Seems odd asking an HConnection for a ClientProtocol? It feels like a ClientProtocol should have an HConnection? Or that we'd put together a ClientProtocol + an HConnection for it to use in a different object altogether. What you think? bq. > bq. > Maybe this is a question for another client implementation that we do later? But would be interested if you had any ideas on this. I agree a ClientProtocol should have an HConnection. We can hide the HConnection. Given a region location, we can create a ClientProtocol which manages its own HConnection. bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 187 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100874#file100874line187> bq. > bq. > Rename too accordingly. Will fix. bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 565 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100874#file100874line565> bq. > bq. > This is pb VersionedProtocol? So far, it can be any VersionedProtocol: ClientProtocol or HRegionInterface. bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 621 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100874#file100874line621> bq. > bq. > Want to adjust this message? Fixed. bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1440 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100874#file100874line1440> bq. > bq. > This set of protocols is covered by the synchronize held above? i.e. all access on protocols are under a block like this? Good point. Will fix. bq. On 2012-04-11 06:22:10, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1449 bq. > <https://reviews.apache.org/r/4629/diff/2/?file=100874#file100874line1449> bq. > bq. > Is there a difference here? We create an ISA only when we need it previously? Now we create it always? Is there a diff here? HServerAddress is deprecated. We are not going to get an ISA passed in. The protocol is cached so we don't need to create it many times. - Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4629/#review6833 ----------------------------------------------------------- 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