[GitHub] [hbase] rmdmattingly commented on a diff in pull request #5332: [branch-2] HBASE-27657: Connection and Request Attributes

["rmdmattingly (via GitHub)" <gi...@apache.org>]

rmdmattingly commented on code in PR #5332:
URL: https://github.com/apache/hbase/pull/5332#discussion_r1272742672


##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTableMultiplexer.java:
##########
@@ -434,7 +434,8 @@ public FlushWorker(Configuration conf, ClusterConnection conn, HRegionLocation a
         HConstants.DEFAULT_HBASE_CLIENT_OPERATION_TIMEOUT);
       // Specify 0 retries in AsyncProcess because we need to reassign puts to different queues
       // if regions are moved.
-      this.ap = new AsyncProcess(conn, conf, rpcCallerFactory, rpcControllerFactory, 0);
+      this.ap = new AsyncProcess(conn, conf, rpcCallerFactory, rpcControllerFactory, 0,
+        Collections.emptyMap());

Review Comment:
   do we need request attribute support here? Seems like maybe no since it is deprecated



##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncProcess.java:
##########
@@ -153,12 +153,13 @@ public void waitUntilDone() throws InterruptedIOException {
   private final int periodToLog;
 
   AsyncProcess(ClusterConnection hc, Configuration conf, RpcRetryingCallerFactory rpcCaller,
-    RpcControllerFactory rpcFactory) {
-    this(hc, conf, rpcCaller, rpcFactory, hc.getConnectionConfiguration().getRetriesNumber());
+    RpcControllerFactory rpcFactory, Map<String, byte[]> requestAttributes) {

Review Comment:
   The changes to AsyncProcess are a little ugly imo, but this was the most straightforward way to get request attribute support in HTable



##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BufferedMutatorImpl.java:
##########
@@ -142,7 +142,9 @@ public class BufferedMutatorImpl implements BufferedMutator {
     RpcControllerFactory rpcFactory, BufferedMutatorParams params) {
     this(conn, params,
       // puts need to track errors globally due to how the APIs currently work.
-      new AsyncProcess(conn, conn.getConfiguration(), rpcCallerFactory, rpcFactory));
+      // todo rmattingly support buffered mutator request attributes
+      new AsyncProcess(conn, conn.getConfiguration(), rpcCallerFactory, rpcFactory,
+        Collections.emptyMap()));

Review Comment:
   I think it was an oversight not to implement request attribute support for the buffered mutator in https://github.com/apache/hbase/pull/5326. I can introduce that here now and then port to master



##########
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcControllerImpl.java:
##########
@@ -281,4 +282,15 @@ public String toString() {
       + exception + ", regionInfo=" + regionInfo + ", priority=" + priority + ", cellScanner="
       + cellScanner + '}';
   }
+
+  public static void configureRequestAttributes(RpcController rpcController,
+    Map<String, byte[]> requestAttributes) {
+    if (
+      !requestAttributes.isEmpty() && rpcController != null
+        && rpcController instanceof HBaseRpcController
+    ) {
+      HBaseRpcController controller = (HBaseRpcController) rpcController;
+      controller.setRequestAttributes(requestAttributes);
+    }
+  }

Review Comment:
   I think we could move this to the RpcControllerFactory; I originally put this here because I thought we'd need to configure request attributes in several places before finding the AsyncProcess class



##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRequestAndConnectionAttributes.java:
##########
@@ -224,6 +224,27 @@ public void testRequestAttributesMultiPut() throws IOException {
     assertTrue(REQUEST_ATTRIBUTES_VALIDATED.get());
   }
 
+  @Test
+  public void testRequestAttributesCheckAndMutate() throws IOException {

Review Comment:
   Just thought this would be nice extra test coverage in branch-2



-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org