bbeaudreault commented on code in PR #5366:
URL: https://github.com/apache/hbase/pull/5366#discussion_r1302015880
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRequestAndConnectionAttributes.java:
##########
@@ -118,6 +126,15 @@ public void testConnectionAttributes() throws IOException {
}
}
+ private void ensureBuffersAreOverwritten(Table table, byte[] cf) throws
IOException {
+ // this will cause unread connection attributes on the
Review Comment:
This comment seems unfinished, but to simplify this I think you can get the
same effect by just using a large rowkey on the get. I reproduced with a 300
byte random rowkey in the get above.
As we discussed, this involves only netty's allocator. What I think is
happening here is that the connection header ends up being backed by a buffer
which is around 371 bytes (from debugger). The attributes exist at some offset
in in that buffer, towards the end since the field is new on the proto
messasge. A single Get with a small int-sized rowkey and no filters ends up
only ~120 bytes (also from debugger). Since nothing else is happening on the
server, when the Get comes it it does corrupt the connection header buffer but
only the first 120 bytes. If you use a rowkey of 300 bytes the overall payload
of 120 becomes closer to 420 which is large enough that the corruption extends
into the range of affecting the attributes.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCall.java:
##########
@@ -83,7 +83,7 @@ public interface RpcCall extends RpcCallContext {
/** Returns The request header of this call. */
RequestHeader getHeader();
- ConnectionHeader getConnectionHeader();
Review Comment:
While we are here, I wonder if we should retain parity with request
attributes. We could add a `Map<String, byte[]> getRequestAttributes()`. I
think the impl for that should be lazy since there's no point spending
cpu/memory creating the map if it's not going to be used. You can use this in
your slow logs PR, or you could add it in that PR if you want. I just want to
try to see if we can get it in before 2.6.0 drops.
Please add javadoc to this new method
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java:
##########
@@ -405,6 +409,9 @@ private CodedInputStream createCis(ByteBuff buf) {
// Reads the connection header following version
private void processConnectionHeader(ByteBuff buf) throws IOException {
this.connectionHeader = ConnectionHeader.parseFrom(createCis(buf));
+ this.connectionAttributes = connectionHeader.getAttributeList().stream()
Review Comment:
lets initialize the map with an explicit size. and set it to null or
Collections.emptyMap() if there are no attributes on the header.
it's good to keep things sized int his context because a regionserver may
have many open connections and a little wasted memory per connection can add up
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRequestAndConnectionAttributes.java:
##########
@@ -84,6 +86,8 @@ public class TestRequestAndConnectionAttributes {
@BeforeClass
public static void setUp() throws Exception {
TEST_UTIL = new HBaseTestingUtil();
+ Configuration conf = TEST_UTIL.getConfiguration();
+ conf.setBoolean(ALLOCATOR_POOL_ENABLED_KEY, true);
Review Comment:
i dont think we need this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
