Review Board 1.7.22


Patch for HBASE-6414

Review Request #6491 - Created Aug. 8, 2012 and updated

Devaraj Das
trunk
HBASE-6414
Reviewers
jxiang, stack, tedyu
hbase
Attached is a patch that passes unit tests.

Added unit tests for the HRegionServer.QosFunction that has changed significantly.
Unit tests pass.
Total:
34
Open:
30
Resolved:
4
Dropped:
0
Status:
From:
Description From Last Updated Status
Should we call it RpcRequest, RpcResponse and RpcHeader instead? Jimmy Xiang Aug. 9, 2012, 12:08 a.m. Open
Can we share the header and call them RpcHeader? Jimmy Xiang Aug. 9, 2012, 12:08 a.m. Open
Should we limit the size and make sure the size is not too big? Jimmy Xiang Aug. 9, 2012, 12:08 a.m. Open
Does the requestClassName go together with request? Is it ok to have request, but no requestClassName? Jimmy Xiang Aug. 9, 2012, 5:14 p.m. Open
Do you think this method belongs to RpcServer ? The test can call getRpcServer().getQosFunction(). Ted Yu Aug. 9, 2012, 8:15 p.m. Open
Great Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Do we have to do this? Does the calcuation of the serialized size internally serialize the pb? If so, it ... Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Why we do this on call.param? Aren't we serializing the call? This is probably a holdover from the old stuff? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Great Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Should there a description of the new rpc protocol? The header then the pb? Or you think it just too ... Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Same comment as above... are we possibly calculating 'size' of serialized message twice in here or doe pb remember it? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
param is bad name for this variable Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Is this a good idea? Making it static? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Why we have to set class on a pb? Does it need this deserializing? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
This is a static map caching returning types vs method names? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Thats a nice purge. Michael Stack Aug. 12, 2012, 10:28 p.m. Open
What we keeping in here? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Add line either side of this method so it looks like a method. Michael Stack Aug. 12, 2012, 10:28 p.m. Open
These statics are a good idea? In a JVM running multiple servers? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
This was there before? It was moved from elsewhere or did you add it? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Is this a good idea? Previously we made it per invocation? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
You had to redo this when you made it pb only? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Oh, is this why you send the classname as part of rpc? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Can this be static? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
We can't pass this in on construction? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Is this double deserialization? Here we deserialize the whole thing after undoing a few Strings (region and method name?) Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Its not possible for argumentClasses to clash? Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Ok. This is good. Ignore comment above about describing the protocol in rpc classes Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Ok. I suppose its not possible for argument names to clash if these are what they are. Can you deserialize ... Michael Stack Aug. 12, 2012, 10:28 p.m. Open
This test is missing something like @org.junit.Rule public org.apache.hadoop.hbase.ResourceCheckerJUnitRule cu = new org.apache.hadoop.hbase.ResourceCheckerJUnitRule(); from the bottom. W/o it, the categorization ... Michael Stack Aug. 12, 2012, 10:28 p.m. Open
Review request changed
Updated (Aug. 18, 2012, 12:53 a.m.)
Addresses the last comment from Stack.
Ship it!
Posted (Aug. 18, 2012, 10:19 p.m.)
+1  Lets get it in.  We can look for double deserializations if any, etc., once its in.  Attach v7 to JIRA so we can hadoopqa it DD.  Good stuff.