[ https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249006#comment-13249006 ]
jirapos...@reviews.apache.org commented on HBASE-5620: ------------------------------------------------------ bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > Good stuff Jimmy. A few comments below. Sorry I ramble at times. Read to the end before reacting because I change my mind as I work through the patch. You forgot to add RequestConverter? Thanks for reviewing. RequestConverter is the 21st file. There are more files on page 2. Please review them too. Thanks. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 766 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98832#file98832line766> bq. > bq. > white space Will remove. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/protobuf/RegionAdminProtocol.java, line 37 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98833#file98833line37> bq. > bq. > Is this just regionadmin protocol? Or is it general admin? Should I change it to AdminProtocol? There is no other admin. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 510 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98832#file98832line510> bq. > bq. > Its as though we should give clients a way to pass in the raw pbs rather than have them go via our versions of Get, Delete, etc. I agree. We can add some new methods later. For now, I'd like to make the existing functions work at first. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 160 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98832#file98832line160> bq. > bq. > Ugh. KeyValue needs to be an Interface that both pb and our current KV implements. bq. > bq. > This will be a performance killer? Should Result let out the raw pb KV? I was thinking to has some new methods which use raw pbs, which old clients should migrate to. Let me change the current KeyValue class to an interface. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line 149 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98832#file98832line149> bq. > bq. > Why not throw ServiceException? I like to throw ServiceException too. Let me give it a try. If the existing interface is not affected, I will do it. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java, line 418 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98830#file98830line418> bq. > bq. > Is ServiceException a pb exception? It is from pb. But it is not a pb message as I know. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/ipc/RegionProtocols.java, line 36 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98829#file98829line36> bq. > bq. > What is this for? Region server could implement multiple protocols such as RegionClientProtocol and RegionAdminProtocol. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/ipc/RegionProtocols.java, line 27 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98829#file98829line27> bq. > bq. > Fix Fixed. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java, line 363 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98825#file98825line363> bq. > bq. > Do you have to? This stuff is nasty enough w/o lettting it out of this package. It is to support Filters and comparators which are still Writable. We can change it back once Filters and comparators are moved to pb too. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java, line 230 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98822#file98822line230> bq. > bq. > scannerid is internal or something now? I think it is still public as it is defined in the pb messages. For ScannerCallable, it is internal though. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java, line 192 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98822#file98822line192> bq. > bq. > I suppose HBaseClient down in rpc knows it because it has to deserialize the pb? You are right. We need to come up a way to pop this info up. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1441 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98820#file98820line1441> bq. > bq. > Does this need to be synchronized at all? I think so. It's better to be synchronized. The connection is shared. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1428 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98820#file98820line1428> bq. > bq. > Is this good? Should we have servername? What if server restarts in between? Would we want to notice that we don't have a connection to new instantiation and go create one? In this case, they need to close the existing connection, and get a new one. Otherwise, there will be memory leak. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 606 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98820#file98820line606> bq. > bq. > Move these defines into this class rather than up in HConstants? Ok, will do. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1395 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98820#file98820line1395> bq. > bq. > Whats happening here? We want to drop the methods that took hostname and port only and move to those that take servername? Just did some refactory so that we can use the same method to get a proxy for either a RegionClientProtocol and a HRegionInterface. The public interface is not touched. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 43 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98819#file98819line43> bq. > bq. > Why do we need to pollute HConnection w/ higher level notions such as RegionClientProtocol (or ClientProtocol?).... hmmm ClientProtocol would be ok but not RegionClientProtocol... OK, will rename it to ClientProtocol. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 530 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98818#file98818line530> bq. > bq. > Whats the NULL_CONTROLLER about? Pb rpc needs a RpcController. Let me use null directly. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 574 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98818#file98818line574> bq. > bq. > So we don't close scanners anymore? That's right, if closeScanner is specified in the request. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 525 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98818#file98818line525> bq. > bq. > Why we need a converter? Pb needs some work to build a message. A converter is some helper to share some logic. The ProtobufUtil will be too big soon. That's why I have this separate converter. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 518 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98818#file98818line518> bq. > bq. > this should be getClient? We're going to get a client for a table? Maybe should be getTableClient? The region client protocol. How about just getClient? bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 517 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98818#file98818line517> bq. > bq. > RegionClientProtocol is a bad name, no? Should it be just ClientProtocol and AdminProtocol? Ok, will change to ClientProtocol and AdminProtocol. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 661 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98815#file98815line661> bq. > bq. > Should these be in here? Can they be elsewhere, as defines in RegionClassProtocol? Will move to HConnectionManager. bq. On 2012-04-06 21:02:20, Michael Stack wrote: bq. > src/main/java/org/apache/hadoop/hbase/client/HConnection.java, line 234 bq. > <https://reviews.apache.org/r/4629/diff/1/?file=98819#file98819line234> bq. > bq. > I wonder if this needs to be public and in the Interface? Could it be an internal thing? How uses this thing that comes out? Maybe it needs to be here but sure seems like an internal thing: i.e. we pass hostname and port of a particular regionserver but then internally as we go about the cluster the client will go to new regionservers and set up connections.... bq. > bq. > Looking around though, it seems that while most uses of this method should be shut down: e.g. over in catalog package and replaced by calls that go via HTable, there are a few places we need this still probably .. as in the master sendRegionClose, etc. calls. So you can't get rid of it. bq. > bq. > But this looks like it should be called RegionServerProtocol bq. > or ClientProtocol and not RegionClientProtocol. It connects to a RegionServer not a Region..... bq. > bq. > Or what you think Jimmy? When I look at the RegionClient proto file, is it true to say that in each message, we MUST pass a region since we are going against a particular region... which would mean this is indeed a RegionClientProtocol (you just have to specify coordinates in two steps; 1. pass servername setting up connection, then 2. for any call on the connection, need to specify the region.....) bq. > bq. > bq. > Agreed to rename it to ClientProtocol. - Jimmy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4629/#review6751 ----------------------------------------------------------- On 2012-04-03 23:32:10, 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-03 23:32:10) 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/HConstants.java 21ac4ba 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 aa30969 bq. src/main/java/org/apache/hadoop/hbase/client/HTable.java 3db0295 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/RegionProtocols.java PRE-CREATION 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/ProtobufUtil.java 2eb57de bq. src/main/java/org/apache/hadoop/hbase/protobuf/RegionAdminProtocol.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/protobuf/RegionClientProtocol.java PRE-CREATION 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/HBaseProtos.java 4026da0 bq. src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java b36a9c0 bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 4f80999 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/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 d2b3060 bq. src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36 bq. src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java 227c5f2 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