ndimiduk commented on code in PR #6076:
URL: https://github.com/apache/hbase/pull/6076#discussion_r1680690629
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncTableImpl.java:
##########
@@ -214,8 +214,8 @@ private <T> SingleRequestCallerBuilder<T> newCaller(byte[]
row, int priority, lo
.operationTimeout(operationTimeoutNs, TimeUnit.NANOSECONDS)
.pause(pauseNs, TimeUnit.NANOSECONDS)
.pauseForServerOverloaded(pauseNsForServerOverloaded,
TimeUnit.NANOSECONDS)
- .maxAttempts(maxAttempts).setRequestAttributes(requestAttributes)
-
.startLogErrorsCnt(startLogErrorsCnt).setRequestAttributes(requestAttributes);
+ .maxAttempts(maxAttempts).startLogErrorsCnt(startLogErrorsCnt)
Review Comment:
Haha, oops.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncBufferedMutatorBuilder.java:
##########
@@ -38,6 +39,16 @@ public interface AsyncBufferedMutatorBuilder {
*/
AsyncBufferedMutatorBuilder setRpcTimeout(long timeout, TimeUnit unit);
+ /**
+ * Set a rpc request attribute.
+ */
+ AsyncBufferedMutatorBuilder setRequestAttribute(String key, byte[] value);
Review Comment:
Do we expect this to override all existing requestAttributes and replace the
collection with this value? Or, do we expect this method to add an additional
requestAttribute to an existing set?
Take a look at the breadth of API exposed on `Immutable.Builder`
implementations around collection objects --
https://immutables.github.io/immutable.html#array-collection-and-map-attributes
I'm not saying that we need all of these, but at least consider which semantics
we want to support and why.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRequestAndConnectionAttributes.java:
##########
@@ -230,6 +235,47 @@ public void testRequestAttributesMultiPut() throws
IOException {
assertTrue(REQUEST_ATTRIBUTES_VALIDATED.get());
}
Review Comment:
I know it's not your test, but all these mutable static fields are really
problematic...
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncBufferedMutatorBuilder.java:
##########
@@ -38,6 +39,16 @@ public interface AsyncBufferedMutatorBuilder {
*/
AsyncBufferedMutatorBuilder setRpcTimeout(long timeout, TimeUnit unit);
+ /**
+ * Set a rpc request attribute.
+ */
+ AsyncBufferedMutatorBuilder setRequestAttribute(String key, byte[] value);
Review Comment:
Ah, or maybe you're hamstrung by the apis that TableBuilder exposes? Maybe
we should look at expanding the scope of these methods in a separate JIRA.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncBufferedMutator.java:
##########
@@ -93,4 +94,11 @@ default CompletableFuture<Void> mutate(Mutation mutation) {
default long getPeriodicalFlushTimeout(TimeUnit unit) {
throw new UnsupportedOperationException("Not implemented");
}
+
+ /**
+ * Returns the rpc request attributes.
+ */
+ default Map<String, byte[]> getRequestAttributes() {
+ throw new UnsupportedOperationException("Not implemented");
Review Comment:
Why does this throw instead of returning an empty map? Do we require
downstream implementations to explicitly implement something?
--
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]
