[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13542245#comment-13542245
 ] 

stack commented on HBASE-5620:
--

[~jxiang] I've been experimenting over in HBASE-6521  RpcController seems 
pretty useless though experimenting I ended up using it to carry extra payload 
in and out of the method on the server-side, not in client as here.

VersionedProtocol is going away.  We could just resort to raw 
BlockingInterfaces everywhere instead (though I do not think this will be work 
too cleanly since there is no common ancestor to BlockingInterface).  We could 
redefine all methods to be what is in BlockingInterface minus this 
rpccontroller stuff -- or, as you have it, just use what protoc generates as 
the Interface and be clear that the rpccontroller goes unused.

I think we should go w/ the latter approach.  Use raw blockinginterface where 
we can being clear rpccontroller is an unused field.

Thanks for the answers Jimmy.

> 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/RPC, master, migration, regionserver
>Reporter: Jimmy Xiang
>Assignee: Jimmy Xiang
> Fix For: 0.96.0
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13542235#comment-13542235
 ] 

Jimmy Xiang commented on HBASE-5620:


I thought about that. It is a clean choice to redefine the interface. I was 
wondering we may be able to use RpcController somehow in the future if we need 
to do something to the RPC engine.

@Stack, if we are sure we are not going to use RpcController, we can redefine 
the interface and now is the time.

> 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/RPC, master, migration, regionserver
>Reporter: Jimmy Xiang
>Assignee: Jimmy Xiang
> Fix For: 0.96.0
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13540670#comment-13540670
 ] 

stack commented on HBASE-5620:
--

[~jxiang] BlockingInterface has references to RpcController in it, a facility 
we make no use of. For example, here is the get from the Interface:

{code}
  public 
org.apache.hadoop.hbase.protobuf.generated.ClientProtos.GetResponse get(
  com.google.protobuf.RpcController controller,
  org.apache.hadoop.hbase.protobuf.generated.ClientProtos.GetRequest 
request)
  throws com.google.protobuf.ServiceException;
{code}

Whenever we use the method, we always pass null for the RpcController as in:

{code}
  GetResponse response = client.get(null, request);
{code}

Were you thinking this was ok price to pay for not redefining the Interface 
minus mention of RpcProtocol?  Thanks.

> 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/RPC, master, migration, regionserver
>Reporter: Jimmy Xiang
>Assignee: Jimmy Xiang
> Fix For: 0.96.0
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13540656#comment-13540656
 ] 

Jimmy Xiang commented on HBASE-5620:


There is no special reason other than that our RPC engine uses blocking call.

As to @TokenInfo, @KerberosInfo, they are inherited from the existing old 
HRegionInterface.

> 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/RPC, master, migration, regionserver
>Reporter: Jimmy Xiang
>Assignee: Jimmy Xiang
> Fix For: 0.96.0
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13540644#comment-13540644
 ] 

stack commented on HBASE-5620:
--

[~jxiang] In ClientProtocol (and AdminProtocol, etc.), was there a particular 
reason for implementing BlockingInterface?  See below:

{code}
+/**
+ * Protocol that a HBase client uses to communicate with a region server.
+ */
+@KerberosInfo(
+  serverPrincipal = "hbase.regionserver.kerberos.principal")
+@TokenInfo("HBASE_AUTH_TOKEN")
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public interface ClientProtocol extends
+ClientService.BlockingInterface, VersionedProtocol {
+  public static final long VERSION = 1L;
+}
{code}

For the annotations, @TokenInfo, @KerberosInfo, what made you add these?

Thanks Jimmy.  Just trying to figure out how we arrived at what we have in our 
Protocols.

> 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/RPC, master, migration, regionserver
>Reporter: Jimmy Xiang
>Assignee: Jimmy Xiang
> Fix For: 0.96.0
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13263742#comment-13263742
 ] 

Jimmy Xiang commented on HBASE-5620:


Yes, I was thinking to eliminate the conversion layer if possible, probably 
starting from the server side.
As to I performance test I did, I think the 27us overhead is more meaningful 
than the percentage number.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Zhihong Yu (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13262742#comment-13262742
 ] 

Zhihong Yu commented on HBASE-5620:
---

Is there plan to adopt various measures to counter this 8% performance dip ?

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13262216#comment-13262216
 ] 

Jimmy Xiang commented on HBASE-5620:


You are right.  I used just one thread. This can give us a clear picture on pb 
rpc overhead, right?

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Todd Lipcon (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13262203#comment-13262203
 ] 

Todd Lipcon commented on HBASE-5620:


That sounds like you only ran YCSB with 1 thread...? 1 sec / 2725 = 366us.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13262193#comment-13262193
 ] 

Jimmy Xiang commented on HBASE-5620:


Yes, the throughput is also degraded.  Without the patch, it is 2941 ops/sec; 
with the patch it is 2725 ops/sec, about 7.4% degrade.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Todd Lipcon (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13262176#comment-13262176
 ] 

Todd Lipcon commented on HBASE-5620:


Hey Jimmy. Any difference on throughput?

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13262017#comment-13262017
 ] 

Jimmy Xiang commented on HBASE-5620:


I did some testing with YCSB.  I loaded 9000 rows to a table and read (Get) 
from it for 200 times.
The idea is to have less data so that all are in memory, so as to test the RPC 
overhead.

The performance number changes.  On average, the average read latency is 335us 
without the patch, 362us with the patch.
It is about 27us (8%) degrade.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Hudson (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13257264#comment-13257264
 ] 

Hudson commented on HBASE-5620:
---

Integrated in HBase-TRUNK-security #175 (See 
[https://builds.apache.org/job/HBase-TRUNK-security/175/])
HBASE-5810 HBASE-5620 Convert the client protocol of HRegionInterface to PB 
addendum (Revision 1327629)

 Result = FAILURE
stack : 
Files : 
* 
/hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java


> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Hudson (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13256897#comment-13256897
 ] 

Hudson commented on HBASE-5620:
---

Integrated in HBase-TRUNK #2781 (See 
[https://builds.apache.org/job/HBase-TRUNK/2781/])
HBASE-5810 HBASE-5620 Convert the client protocol of HRegionInterface to PB 
addendum (Revision 1327629)

 Result = FAILURE
stack : 
Files : 
* 
/hbase/trunk/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureRpcEngine.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RpcEngine.java
* /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java


> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13255778#comment-13255778
 ] 

stack commented on HBASE-5620:
--

I made HBASE-5810 to apply this Jimmy.   Good stuff.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13255704#comment-13255704
 ] 

Jimmy Xiang commented on HBASE-5620:


@Stack, not every invocation will throw an exception.  In case it throws an 
exception, it should be a ServiceException for pb.  It used to be IOException.  
Without the change, for pb calls, it won't get a ServiceException in case 
something goes wrong.  It gets an undeclared exception
with the cause to be an IOE, and the upper layer doesn't know how to handle it.

The Set in Invocation is used to decide if a protocol a pb one, so 
ServiceException should be used.  I put it there because it is
used for both WritableRpcEngine and SecureRpcEngine.


> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13255667#comment-13255667
 ] 

stack commented on HBASE-5620:
--

@Jimmy So every invocation will throw an exception?

{code}
+// For protobuf protocols, ServiceException is expected
{code}

Whats the Set in Invocation doing?   You add it but don't seem to access it?

I like the removal of a call method down through the rpc stack

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Zhihong Yu (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=1324#comment-1324
 ] 

Zhihong Yu commented on HBASE-5620:
---

@Stack:
Do you think the addendum is ready to be integrated ?

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13254979#comment-13254979
 ] 

Jimmy Xiang commented on HBASE-5620:


I did some testing with YCSB (mostly inserts).  It gave me better performance 
for the patch which was a surprise to me.
I will do some read-only testing with YCSB too.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Todd Lipcon (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13254961#comment-13254961
 ] 

Todd Lipcon commented on HBASE-5620:


Did anyone do any benchmarks here? It concerns me to have this committed 
without even knowing how it affects performance.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13254360#comment-13254360
 ] 

Jimmy Xiang commented on HBASE-5620:


Thanks for reviewing. Both regular test suite and security test suite are green 
for me. I mean all tests in the suite.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Zhihong Yu (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13254258#comment-13254258
 ] 

Zhihong Yu commented on HBASE-5620:
---

Addendum makes sense.

TestAccessControlFilter passes with the addendum.
Whole security test suite should be run.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13254216#comment-13254216
 ] 

Jimmy Xiang commented on HBASE-5620:


TestForceCacheImportantBlocks is green for me.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Hadoop QA (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13254215#comment-13254215
 ] 

Hadoop QA commented on HBASE-5620:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12522687/hbase-5620-sec.patch
  against trunk revision .

+1 @author.  The patch does not contain any @author tags.

-1 tests included.  The patch doesn't appear to include any new or modified 
tests.
Please justify why no new tests are needed for this 
patch.
Also please list what manual steps were performed to 
verify this patch.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

-1 findbugs.  The patch appears to introduce 3 new Findbugs (version 1.3.9) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

 -1 core tests.  The patch failed these unit tests:
   
org.apache.hadoop.hbase.io.hfile.TestForceCacheImportantBlocks

Test results: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1527//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1527//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1527//console

This message is automatically generated.

> 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
>
> Attachments: hbase-5620-sec.patch, hbase-5620_v3.patch, 
> hbase-5620_v4.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13254153#comment-13254153
 ] 

Jimmy Xiang commented on HBASE-5620:


@Stack, thanks.

@Ted, I am looking into it now.

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253975#comment-13253975
 ] 

stack commented on HBASE-5620:
--

@Jimmy I think this is the biggest patch ever applied to HBase.  Congrats!

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253974#comment-13253974
 ] 

stack commented on HBASE-5620:
--

@Jimmy I think this is the biggest patch ever applied to HBase.  Congrats!

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253852#comment-13253852
 ] 

Jimmy Xiang commented on HBASE-5620:


@Stack, thanks a lot!

I moved them to top-level in HBase-5621 and posted a review request. Could you 
please review?
I am ok to move them to client package.

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253843#comment-13253843
 ] 

stack commented on HBASE-5620:
--

I did too and trunk is also no longer complaining.  The rat test is a PITA.  
There was probably some deitrus laying around that it picked up.  I modified 
the trunk build to keep the rat.txt report next time.

@Jimmy I think top-level is better than where it currently is.  What other 
Protocols would go up to the top level?  None I suppose.  I suppose they should 
be in client package but its a little perverse having the the client stuff 
reaching into zk and util and protobuf... 

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253840#comment-13253840
 ] 

Jimmy Xiang commented on HBASE-5620:


I ran Apache Rat check like mvn apache-rat:check, and it is ok.

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Zhihong Yu (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253802#comment-13253802
 ] 

Zhihong Yu commented on HBASE-5620:
---

Here is the command Jenkins used:
{code}
[trunk] $ /home/hudson/tools/maven/latest3/bin/mvn -e -X clean 
-Dmaven.test.redirectTestOutputToFile=true site install assembly:single 
-DskipITs -Prelease
{code}

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253799#comment-13253799
 ] 

Jimmy Xiang commented on HBASE-5620:


@Stack, I will move ClientProtocol.java and AdminProtocol.java to top level in 
HBASE-5621 since they are common.  I added HBASE-5785 to track the unit test 
issue.

@Ted, can I check the licenses without doing a release build?

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Zhihong Yu (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253781#comment-13253781
 ] 

Zhihong Yu commented on HBASE-5620:
---

Looks like trunk build failed with:
{code}
[ERROR] Failed to execute goal org.apache.rat:apache-rat-plugin:0.8:check 
(default) on project hbase: Too many unapproved licenses: 1 -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal 
org.apache.rat:apache-rat-plugin:0.8:check (default) on project hbase: Too many 
unapproved licenses: 1
{code}


> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253714#comment-13253714
 ] 

stack commented on HBASE-5620:
--

Mind opening new issues Jimmy to do outstanding work like unit tests?

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253712#comment-13253712
 ] 

stack commented on HBASE-5620:
--

I think  src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java is 
in wrong package.  Ditto for AdminProtocol.  What you think Jimmy?  Should we 
move them?  Where should they go?  At top level?  Or into client package?


> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[stack (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253708#comment-13253708
 ] 

stack commented on HBASE-5620:
--

It passed for me.  Let me commit this monster.

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253538#comment-13253538
 ] 

Jimmy Xiang commented on HBASE-5620:


TestWALPlayer passed for me.  I didn't have the latest from trunk?

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Hadoop QA (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253513#comment-13253513
 ] 

Hadoop QA commented on HBASE-5620:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12522579/hbase-5620_v4.patch
  against trunk revision .

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 30 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

-1 findbugs.  The patch appears to introduce 3 new Findbugs (version 1.3.9) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

 -1 core tests.  The patch failed these unit tests:
   org.apache.hadoop.hbase.mapreduce.TestWALPlayer

Test results: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1511//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1511//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1511//console

This message is automatically generated.

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Jimmy Xiang (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253377#comment-13253377
 ] 

Jimmy Xiang commented on HBASE-5620:


I will take a look at the test failures.

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Hadoop QA (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13253169#comment-13253169
 ] 

Hadoop QA commented on HBASE-5620:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12522531/hbase-5620_v4.patch
  against trunk revision .

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 30 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

-1 findbugs.  The patch appears to introduce 3 new Findbugs (version 1.3.9) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

 -1 core tests.  The patch failed these unit tests:
   
org.apache.hadoop.hbase.io.hfile.TestForceCacheImportantBlocks
  org.apache.hadoop.hbase.replication.TestReplication
  org.apache.hadoop.hbase.mapreduce.TestWALPlayer
  org.apache.hadoop.hbase.regionserver.wal.TestHLogSplit
  org.apache.hadoop.hbase.replication.TestMultiSlaveReplication
  org.apache.hadoop.hbase.regionserver.wal.TestHLog
  org.apache.hadoop.hbase.replication.TestMasterReplication

Test results: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1508//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1508//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1508//console

This message is automatically generated.

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch, 
> hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Hadoop QA (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252990#comment-13252990
 ] 

Hadoop QA commented on HBASE-5620:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12522499/hbase-5620_v4.patch
  against trunk revision .

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 30 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

-1 findbugs.  The patch appears to introduce 3 new Findbugs (version 1.3.9) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

 -1 core tests.  The patch failed these unit tests:
   org.apache.hadoop.hbase.regionserver.wal.TestHLogSplit
  org.apache.hadoop.hbase.replication.TestMultiSlaveReplication
  org.apache.hadoop.hbase.regionserver.wal.TestHLog
  org.apache.hadoop.hbase.replication.TestMasterReplication

Test results: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1501//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1501//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1501//console

This message is automatically generated.

> 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
>
> Attachments: hbase-5620_v3.patch, hbase-5620_v4.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252936#comment-13252936
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/
---

(Updated 2012-04-12 22:33:53.615686)


Review request for hbase.


Changes
---

Minor code change.  Fixed a bug.


Summary
---

This is the client protocol part of region interface. The admin protocol part 
will be done in a different jira.

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

Filters, comparators and coprocessor parameters are still Writable.  They will 
be addressed in different jiras.

The existing client interface is not changed so that we don't break existing 
clients.


This addresses bug HBASE-5620.
https://issues.apache.org/jira/browse/HBASE-5620


Diffs (updated)
-

  src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java 0129ee9 
  src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 3167f23 
  src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 16e4017 
  src/main/java/org/apache/hadoop/hbase/client/HConnection.java 5d43086 
  src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 0b783ce 
  src/main/java/org/apache/hadoop/hbase/client/HTable.java aa7652f 
  src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java 9903df3 
  src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java ddcf9ad 
  src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java 1acbdab 
  src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 
  src/main/java/org/apache/hadoop/hbase/io/TimeRange.java d135393 
  src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 05ae717 
  src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 
  src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 0573c68 
  src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java 
b71ae66 
  src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 2eb57de 
  src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java 
4026da0 
  
src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionAdminProtos.java 
2169310 
  
src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java
 b36a9c0 
  src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 9b34e61 
  src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 
703e73d 
  src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java b520f3f 
  src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 
PRE-CREATION 
  src/main/protobuf/Admin.proto PRE-CREATION 
  src/main/protobuf/Client.proto PRE-CREATION 
  src/main/protobuf/RegionAdmin.proto c64d68b 
  src/main/protobuf/RegionClient.proto 358382b 
  src/main/protobuf/hbase.proto da78788 
  src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java c7284dc 
  
src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java
 0042468 
  src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 
e34d8bc 
  src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 
  src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java 41616c8 
  src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 
6ed4ba2 
  src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java b4dcb83 
  src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java ceca6f5 
  src/test/java/org/apache/hadoop/hbase/regionserver/OOMERegionServer.java 
cac2989 
  
src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
 a1bf73b 

Diff: https://reviews.apache.org/r/4629/diff


Testing
---

"mvn -PrunAllTests clean test" is green, except some flaky tests which need to 
run again.

Also tested it on a real cluster with ycsb and bigtop.


Thanks,

Jimmy



> Convert the client protocol of HRegionInterface to PB
> -
>
> Key: H

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[Hadoop QA (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252687#comment-13252687
 ] 

Hadoop QA commented on HBASE-5620:
--

+1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12522452/hbase-5620_v3.patch
  against trunk revision .

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 30 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 1.3.9) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

+1 core tests.  The patch passed unit tests in .

Test results: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1496//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1496//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-HBASE-Build/1496//console

This message is automatically generated.

> 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
>
> Attachments: hbase-5620_v3.patch
>
>


--
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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252623#comment-13252623
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/
---

(Updated 2012-04-12 17:25:51.153610)


Review request for hbase.


Changes
---

Addressed Stack's comments.


Summary
---

This is the client protocol part of region interface. The admin protocol part 
will be done in a different jira.

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

Filters, comparators and coprocessor parameters are still Writable.  They will 
be addressed in different jiras.

The existing client interface is not changed so that we don't break existing 
clients.


This addresses bug HBASE-5620.
https://issues.apache.org/jira/browse/HBASE-5620


Diffs (updated)
-

  src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java 0129ee9 
  src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 3167f23 
  src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 16e4017 
  src/main/java/org/apache/hadoop/hbase/client/HConnection.java 5d43086 
  src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 0b783ce 
  src/main/java/org/apache/hadoop/hbase/client/HTable.java aa7652f 
  src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java 9903df3 
  src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java ddcf9ad 
  src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java 1acbdab 
  src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 
  src/main/java/org/apache/hadoop/hbase/io/TimeRange.java d135393 
  src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 05ae717 
  src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 
  src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 0573c68 
  src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java 
b71ae66 
  src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 2eb57de 
  src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java 
4026da0 
  
src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionAdminProtos.java 
2169310 
  
src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java
 b36a9c0 
  src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 9b34e61 
  src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 
703e73d 
  src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java b520f3f 
  src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 
PRE-CREATION 
  src/main/protobuf/Admin.proto PRE-CREATION 
  src/main/protobuf/Client.proto PRE-CREATION 
  src/main/protobuf/RegionAdmin.proto c64d68b 
  src/main/protobuf/RegionClient.proto 358382b 
  src/main/protobuf/hbase.proto da78788 
  src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java c7284dc 
  
src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java
 0042468 
  src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 
e34d8bc 
  src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 
  src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java 41616c8 
  src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 
6ed4ba2 
  src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java b4dcb83 
  src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java ceca6f5 
  src/test/java/org/apache/hadoop/hbase/regionserver/OOMERegionServer.java 
cac2989 
  
src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
 a1bf73b 

Diff: https://reviews.apache.org/r/4629/diff


Testing
---

"mvn -PrunAllTests clean test" is green, except some flaky tests which need to 
run again.

Also tested it on a real cluster with ycsb and bigtop.


Thanks,

Jimmy



> Convert the client protocol of HRegionInterface to PB
> -
>
> Key: HBASE-

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252622#comment-13252622
 ] 

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?
bq.  
bq.  Jimmy Xiang wrote:
bq.  Could you please check it in if all tests pass?  It can save me some 
rebasing/merging efforts. :)
bq.  
bq.  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.
bq.  I can resolve other issues in HBASE-5621, or separate patches.
bq.

I can.  You going to do another version to address above feedback?


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.  > 
bq.  >
bq.  > Should this class implement Closeable, Stoppable and Abortable?
bq.  
bq.  Jimmy Xiang wrote:
bq.  RegionServerServices defined Stoppable, Abortable. Now RegionServer 
implements RegionServerServices.
bq.  Not sure about Closeable.

Good


- Michael


---
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/Regi

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252619#comment-13252619
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--



bq.  On 2012-04-12 00:03:44, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java, 
line 187
bq.  > 
bq.  >
bq.  > Do these have unit tests?  Looks like it would be easy enough to add 
and could find some nasty bugs early.
bq.  
bq.  Jimmy Xiang wrote:
bq.  Not for now, I can add it in a separate patch if it is ok with you.

Yes.  Make it a blocker though.  I see unit tests finding bugs in this stuff.


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6863
---


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/TestMetaR

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
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.  > 
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.  > 
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.  > 
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.  > 
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/HbaseObjectWr

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252613#comment-13252613
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--



bq.  On 2012-04-12 00:03:44, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java, 
line 187
bq.  > 
bq.  >
bq.  > Do these have unit tests?  Looks like it would be easy enough to add 
and could find some nasty bugs early.

Not for now, I can add it in a separate patch if it is ok with you.


bq.  On 2012-04-12 00:03:44, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java, line 
28
bq.  > 
bq.  >
bq.  > This is the right comment?

Fixed.


bq.  On 2012-04-12 00:03:44, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java, line 57
bq.  > 
bq.  >
bq.  > Fix this text

Fixed.


bq.  On 2012-04-12 00:03:44, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java, line 
190
bq.  > 
bq.  >
bq.  > Yeah, we were going to try and get the data up from rpc layer

Yes, after changed pb Result to use key value bytes, we should be able to get 
the data now.


bq.  On 2012-04-12 00:03:44, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HTable.java, line 672
bq.  > 
bq.  >
bq.  > Is there an EOFE story here too?   You are dealing w/ that elsewhere?

EOFE is also an IOE.  It should be covered as before.


- Jimmy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6863
---


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.   

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252208#comment-13252208
 ] 

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.
bq.  
bq.  Jimmy Xiang wrote:
bq.  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.  
bq.  Michael Stack wrote:
bq.  Sorry, I don't get the last part?  The response is too hard to make 
when the query is lots of types?
bq.  
bq.  Jimmy Xiang wrote:
bq.  Right, basically the code is not efficient.

Whats your thoughts on making it efficient?  Is it possible?


- Michael


---
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.j

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252197#comment-13252197
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6868
---


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?


src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java


Fix comment.

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.



src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java


Shouldn't this implement Server and RegionServerServices?

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.



src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java


Should this class implement Closeable, Stoppable and Abortable?



src/main/protobuf/Client.proto


Call this keyValueBytes then so method  names give more of a clue as to 
what you'll be getting; save on the surprises?


- Michael


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/HBaseProt

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252072#comment-13252072
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6863
---


I'm halfway down on second page


src/main/java/org/apache/hadoop/hbase/client/HTable.java


Is there an EOFE story here too?   You are dealing w/ that elsewhere?



src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java


Yeah, we were going to try and get the data up from rpc layer



src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java


Does this call  need to return a Result or do we need to add a new method 
to support being able to get results as part of the call to open a scanner?



src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java


Fix this text



src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java


This is the right comment?



src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java


Do these have unit tests?  Looks like it would be easy enough to add and 
could find some nasty bugs early.


- Michael


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

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252035#comment-13252035
 ] 

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.
bq.  
bq.  Jimmy Xiang wrote:
bq.  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.  
bq.  Michael Stack wrote:
bq.  Sorry, I don't get the last part?  The response is too hard to make 
when the query is lots of types?

Right, basically the code is not efficient.


- 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.

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13252005#comment-13252005
 ] 

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.
bq.  
bq.  Jimmy Xiang wrote:
bq.  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.

Sorry, I don't get the last part?  The response is too hard to make when the 
query is lots of types?


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.  > 
bq.  >
bq.  > I said it before but I like this code removal
bq.  
bq.  Jimmy Xiang wrote:
bq.  It is not used so I removed it.

Great


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.  > 
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.
bq.  
bq.  Jimmy Xiang wrote:
bq.  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.

Sounds better


- Michael


---
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/o

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 

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.  > 

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 c

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13251366#comment-13251366
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6833
---


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.


src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java


I said it before but I like this code removal



src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java


What about the EOFE you added today?  Does that need to go in here anywhere?



src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java


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?



src/main/java/org/apache/hadoop/hbase/client/HConnection.java


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?

Maybe this is a question for another client implementation that we do 
later?   But would be interested if you had any ideas on this.



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java


Rename to hbase.clientprotocol.class and CLIENT_PROTOCOL_CLASS.  Seems more 
consistent?



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java


Rename too accordingly.



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java


This is pb VersionedProtocol?



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java


Want to adjust this message?



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java


This set of protocols is covered by the synchronize held above? i.e. all 
access on protocols are under a block like this?



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java


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?


- Michael


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/ja

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249384#comment-13249384
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--



bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, 
line 146
bq.  > 
bq.  >
bq.  > Do you think that we should eventually just deprecate the current 
Get, Delete, and Put, etc. and move to the pb ones altogether?  They are only 
in the way now?
bq.  
bq.  Jimmy Xiang wrote:
bq.  I think so.  We don't want to break the existing applications.

No.  As you suggest, parity first.  Thinking on it, I'm thinking that when we 
write the new client, that it is the new client that just does pbs direct.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, 
line 3195
bq.  > 
bq.  >
bq.  > Some pretty radical removals in here.  For sure we don't need the 
stuff being removed (Otherwise, I'm glad to see it go)
bq.  
bq.  Jimmy Xiang wrote:
bq.  Moved to RegionServer, a new base class.  I was think to remove class 
HRegionServer later on when HRegionInterface is not needed.

Ok.  Would be really cool if there was a RegionServer Interface that the pb 
service implemented (We already have Server Interface and RegionServerServices 
Interface).


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, 
line 98
bq.  > 
bq.  >
bq.  > oh... I think I like what I'm seeing.  Could this be an Interface at 
all?
bq.  
bq.  Jimmy Xiang wrote:
bq.  It is the new pb version of HRegionServer.  So it is a class.

Could it be an Interface that the pb Service implements?  Is that over the top?


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java, 
line 1162
bq.  > 
bq.  >
bq.  > What is the advantage of doing this?
bq.  
bq.  Jimmy Xiang wrote:
bq.  So I don't mess HRegionInterface with the new pb. Later on, it will be 
easier to remove HRegionInterface.

I like this tactic of yours


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6757
---


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.
s

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249382#comment-13249382
 ] 

jirapos...@reviews.apache.org 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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249338#comment-13249338
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/
---

(Updated 2012-04-07 21:30:32.906966)


Review request for hbase.


Changes
---

Addressed Stack's comments.  For KeyValue, I removed it from the pb Result.


Summary
---

This is the client protocol part of region interface. The admin protocol part 
will be done in a different jira.

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

Filters, comparators and coprocessor parameters are still Writable.  They will 
be addressed in different jiras.

The existing client interface is not changed so that we don't break existing 
clients.


This addresses bug HBASE-5620.
https://issues.apache.org/jira/browse/HBASE-5620


Diffs (updated)
-

  src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java 0129ee9 
  src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 97293aa 
  src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 16e4017 
  src/main/java/org/apache/hadoop/hbase/client/HConnection.java 5d43086 
  src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 0b783ce 
  src/main/java/org/apache/hadoop/hbase/client/HTable.java aa7652f 
  src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java 9903df3 
  src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java ddcf9ad 
  src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java 1acbdab 
  src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 
  src/main/java/org/apache/hadoop/hbase/io/TimeRange.java d135393 
  src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 05ae717 
  src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 
  src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 1316457 
  src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java 
19ae18c 
  src/main/java/org/apache/hadoop/hbase/protobuf/AdminProtocol.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/ClientProtocol.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 2eb57de 
  src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java 
4026da0 
  
src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionAdminProtos.java 
2169310 
  
src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java
 b36a9c0 
  src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8a61f7d 
  src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 
703e73d 
  src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java b520f3f 
  src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 
PRE-CREATION 
  src/main/protobuf/Admin.proto PRE-CREATION 
  src/main/protobuf/Client.proto PRE-CREATION 
  src/main/protobuf/RegionAdmin.proto c64d68b 
  src/main/protobuf/RegionClient.proto 358382b 
  src/main/protobuf/hbase.proto da78788 
  src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java c7284dc 
  
src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java
 0042468 
  src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 
e34d8bc 
  src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 
  src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java 41616c8 
  src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 
6ed4ba2 
  src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java b4dcb83 
  src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java ceca6f5 
  src/test/java/org/apache/hadoop/hbase/regionserver/OOMERegionServer.java 
cac2989 
  
src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
 a1bf73b 

Diff: https://reviews.apache.org/r/4629/diff


Testing
---

"mvn -PrunAllTests clean test" is green, except some flaky tests which need to 
run again.

Also tested it on a real cluster with ycsb and bigtop.


Thanks,

Jimmy



> Convert the client protocol of HRegionInterface to PB
> -

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249077#comment-13249077
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--



bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, 
line 89
bq.  > 
bq.  >
bq.  > What does this do?  Needs class comment

Will add comment.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, 
line 97
bq.  > 
bq.  >
bq.  > Spelling

Good catch, will fix.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, 
line 98
bq.  > 
bq.  >
bq.  > We only do the latter if the flag is set, right?

Right.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, 
line 107
bq.  > 
bq.  >
bq.  > Is this a get request builder?  Name method getRequestBuilder?

It uses a request builder to build a get request.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, 
line 146
bq.  > 
bq.  >
bq.  > Do you think that we should eventually just deprecate the current 
Get, Delete, and Put, etc. and move to the pb ones altogether?  They are only 
in the way now?

I think so.  We don't want to break the existing applications.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, 
line 167
bq.  > 
bq.  >
bq.  > buildMutateRequest?

Sounds good to me.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java, 
line 646
bq.  > 
bq.  >
bq.  > Should this stuff be public?  Its internal stuff?  Should it be over 
under the client package?  Then you could keep it encapsulated at least w/i the 
package?  Is it used outside of the client package?

Let me check. I will try not to expose it.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java, 
line 56
bq.  > 
bq.  >
bq.  > Ditto.  Should this be over in the client package?  Is that the only 
place its used?  Could we make it non-public?

Will do if it works.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java, 
line 101
bq.  > 
bq.  >
bq.  > buildActionResult?

Sure.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, 
line 185
bq.  > 
bq.  >
bq.  > We don't need this anymore?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, 
line 193
bq.  > 
bq.  >
bq.  > Why we don't need this anymore?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, 
line 218
bq.  > 
bq.  >
bq.  > We don't need this anymore?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, 
line 229
bq.  > 
bq.  >
bq.  > We don't need any more?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, 
line 271
bq.  > 
bq.  >
bq.  > We don't need this anymore?

Moved to RegionServer, a new base class.


bq.  On 2012-04-06 23:26:21, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServ

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249075#comment-13249075
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--



bq.  On 2012-04-06 23:41:33, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/ipc/RegionProtocols.java, line 36
bq.  > 
bq.  >
bq.  > Where is RegionProtocols used?  We put the two protocols together?  
Do we need to?

Ok, let me remove it.


- Jimmy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6759
---


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.   

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249054#comment-13249054
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6759
---



src/main/java/org/apache/hadoop/hbase/ipc/RegionProtocols.java


Where is RegionProtocols used?  We put the two protocols together?  Do we 
need to?


- Michael


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/re

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13249041#comment-13249041
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6757
---


I missed this second page.  Looks great.  Some radical stuff going on in here.  
Can you explain some?  Good on you  Jimmy.


src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java


What does this do?  Needs class comment



src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java


Spelling



src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java


We only do the latter if the flag is set, right?



src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java


Is this a get request builder?  Name method getRequestBuilder?



src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java


Do you think that we should eventually just deprecate the current Get, 
Delete, and Put, etc. and move to the pb ones altogether?  They are only in the 
way now?



src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java


buildMutateRequest?



src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java


Should this stuff be public?  Its internal stuff?  Should it be over under 
the client package?  Then you could keep it encapsulated at least w/i the 
package?  Is it used outside of the client package?



src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java


Ditto.  Should this be over in the client package?  Is that the only place 
its used?  Could we make it non-public?



src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java


buildActionResult?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


We don't need this anymore?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


Why we don't need this anymore?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


We don't need this anymore?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


We don't need any more?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


We don't need this anymore?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


For sure these are no longer needed?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


Not needed anymore?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


Not used anymore?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


Why this no more?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


Why this no more?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


No more of this?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


Ain't this needed?



src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java


Some pretty radical removals in here.  For sure we don't need the stuff 
being removed (Otherwise, I'm glad to see it go)



src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java


oh... I think I like what I'm seeing.  Could this be an Interface at all?



src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java


What is the advantage of doing this?



src/main/protobuf/RegionClient.proto


We

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 
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.  > 

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13248772#comment-13248772
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/#review6751
---


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?


src/main/java/org/apache/hadoop/hbase/HConstants.java


Should these be in here?  Can they be elsewhere, as defines in 
RegionClassProtocol?



src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java


I don't know how you get away with these removes but I'm glad to see them 
go -- anything that shrinks this catalog package is good by me



src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java


RegionClientProtocol is a bad name, no?  Should it be just ClientProtocol 
and AdminProtocol?



src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java


this should be getClient?  We're going to get a client for a table?  Maybe 
should be getTableClient?



src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java


Why we need a converter?



src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java


Whats the NULL_CONTROLLER about?



src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java


So we don't close scanners anymore?



src/main/java/org/apache/hadoop/hbase/client/HConnection.java


Why do we need to pollute HConnection w/ higher level notions such as 
RegionClientProtocol (or ClientProtocol?) hmmm ClientProtocol would be ok 
but not RegionClientProtocol...



src/main/java/org/apache/hadoop/hbase/client/HConnection.java


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 

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.

But this looks like it should be called RegionServerProtocol
or ClientProtocol and not RegionClientProtocol.  It connects to a 
RegionServer not a Region.

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.)






src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java


Move these defines into this class rather than up in HConstants?



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java


Whats happening here?  We want to drop the methods that took hostname and 
port only and move to those that take servername?



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java


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?



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java


Does this need to be synchronized at all?



src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java


I suppose HBaseClient down in rpc knows it because it has to deserialize 
the pb?



src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java

[jira] [Commented] (HBASE-5620) Convert the client protocol of HRegionInterface to PB

[jirapos...@reviews.apache.org (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/HBASE-5620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13245888#comment-13245888
 ] 

jirapos...@reviews.apache.org commented on HBASE-5620:
--


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4629/
---

Review request for hbase.


Summary
---

This is the client protocol part of region interface. The admin protocol part 
will be done in a different jira.

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

Filters, comparators and coprocessor parameters are still Writable.  They will 
be addressed in different jiras.

The existing client interface is not changed so that we don't break existing 
clients.


This addresses bug HBASE-5620.
https://issues.apache.org/jira/browse/HBASE-5620


Diffs
-

  src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba 
  src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java 0129ee9 
  src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 97293aa 
  src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 16e4017 
  src/main/java/org/apache/hadoop/hbase/client/HConnection.java 5d43086 
  src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java aa30969 
  src/main/java/org/apache/hadoop/hbase/client/HTable.java 3db0295 
  src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java 9903df3 
  src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java ddcf9ad 
  src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java 1acbdab 
  src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java cbfa489 
  src/main/java/org/apache/hadoop/hbase/io/TimeRange.java d135393 
  src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java 05ae717 
  src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java f1f06b0 
  src/main/java/org/apache/hadoop/hbase/ipc/RegionProtocols.java PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 1316457 
  src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java 
19ae18c 
  src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 2eb57de 
  src/main/java/org/apache/hadoop/hbase/protobuf/RegionAdminProtocol.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/RegionClientProtocol.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java 
PRE-CREATION 
  src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java 
4026da0 
  
src/main/java/org/apache/hadoop/hbase/protobuf/generated/RegionClientProtos.java
 b36a9c0 
  src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 4f80999 
  src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java 
703e73d 
  src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java b520f3f 
  src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java 
PRE-CREATION 
  src/main/protobuf/RegionClient.proto 358382b 
  src/main/protobuf/hbase.proto da78788 
  src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java c7284dc 
  
src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditorNoCluster.java
 0042468 
  src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java 
e34d8bc 
  src/test/java/org/apache/hadoop/hbase/io/TestHbaseObjectWritable.java f2f8ee3 
  src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java d2b3060 
  src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 
91dce36 
  src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java 227c5f2 
  src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java ceca6f5 
  src/test/java/org/apache/hadoop/hbase/regionserver/OOMERegionServer.java 
cac2989 
  
src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
 a1bf73b 

Diff: https://reviews.apache.org/r/4629/diff


Testing
---

"mvn -PrunAllTests clean test" is green, except some flaky tests which need to 
run again.

Also tested it on a real cluster with ycsb and bigtop.


Thanks,

Jimmy



> 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.