[
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
