[
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249382#comment-13249382
]
[email protected] commented on HBASE-5620:
------------------------------------------------------
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?
bq.
bq. Jimmy Xiang wrote:
bq. Ok, will change to ClientProtocol and AdminProtocol.
I think thats better.
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?
bq.
bq. Jimmy Xiang wrote:
bq. The region client protocol. How about just getClient?
Yes. In actuality, isn't this really a client on a region so getTableClient
wouldn't be correct either as I suggest above.
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?
bq.
bq. Jimmy Xiang wrote:
bq. 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.
Good. I saw that later on.
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?
bq.
bq. Jimmy Xiang wrote:
bq. Pb rpc needs a RpcController. Let me use null directly.
yeah...
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?
bq.
bq. Jimmy Xiang wrote:
bq. That's right, if closeScanner is specified in the request.
oh, nice.
What about the case where no close is called. We'll hold on to the resources
on the server side w/o cleanup?
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?
bq.
bq. Jimmy Xiang wrote:
bq. Just did some refactory so that we can use the same method to get a
proxy for either a RegionClientProtocol and a HRegionInterface.
bq.
bq. The public interface is not touched.
ok. good.
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?
bq.
bq. Jimmy Xiang wrote:
bq. In this case, they need to close the existing connection, and get a
new one. Otherwise, there will be memory leak.
Otherwise, we will overwrite the old with the new?
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.
bq.
bq. Jimmy Xiang wrote:
bq. 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.
ok
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?
bq.
bq. Jimmy Xiang wrote:
bq. Region server could implement multiple protocols such as
RegionClientProtocol and RegionAdminProtocol.
Do we need to put them together? Who uses this RegionProtocols?
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?
bq.
bq. Jimmy Xiang wrote:
bq. I was thinking to has some new methods which use raw pbs, which old
clients should migrate to.
bq.
bq. Let me change the current KeyValue class to an interface.
That might be hard boss. But, could we implement a subset of KV as an
Interface, enough of a subset that it'd make these pb stuff happy?
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.
bq.
bq. Jimmy Xiang wrote:
bq. I agree. We can add some new methods later. For now, I'd like to
make the existing functions work at first.
Agreed. Looking at the later stuff I think Get, Delete, etc. should just go
and we should have folks use the pb stuff.
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?
bq.
bq. Jimmy Xiang wrote:
bq. Should I change it to AdminProtocol? There is no other admin.
Yes
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6751
-----------------------------------------------------------
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
