HBASE-29850 Add support for dynamic per-request attributes in AsyncTable #7665
Conversation
krconv
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
So the requirements here is to use different request attribute for different requests? I think the design of the request attribute API is for static attributes, if we want to support dynamic attribute, we'd better redesign the APIs, like what you propose here, use a factory. Out of interest, what is your usage for this feature? Thanks. |
|
Yes—our requirement is to set a specific request attribute to a value calculated from thread-local context. We use This is exactly what we do with the |
|
OK, got it. Seems reasonable. So what about this:
WDYT? Thanks. |
|
What do you think of also deprecating |
We can just call request attribute factory's method to get the request attribute for this method? And we can add more javadoc to mention that, since now we allow dynamic request attribute when requesting, the return value may not be the same when you implement a dynamic request attribute factory. |
krconv
force-pushed
the
HBASE-29850-request-attribute-factory
branch
from
38f36f5 to
d708dba
Compare
| * {@link #setRequestAttributesFactory(RequestAttributesFactory)}. | ||
| * @param key the attribute key | ||
| * @param value the attribute value | ||
| * @deprecated Since 3.0.0, will be removed in 4.0.0. Please use |
There was a problem hiding this comment.
I think master is 4.0.0; should it get deprecated for a later version or should I remove it from master?
| private final Map<String, byte[]> requestAttributes = new HashMap<>(); | ||
|
|
||
| /** | ||
| * Sets a request attribute. If value is null, the attribute is removed. |
There was a problem hiding this comment.
The underlying Protobuf ByteString will throw a NPE if value is null, so instead this factory removes the entry if it's null.
| * @see AsyncTableBuilder#setRequestAttributesFactory(RequestAttributesFactory) | ||
| */ | ||
| @InterfaceAudience.Public | ||
| public final class FixedRequestAttributesFactory implements RequestAttributesFactory { |
There was a problem hiding this comment.
The other ideas I had were ImmutableRequestAttributesFactory or StaticRequestAttributesFactory, but I think FixedRequestAttributesFactory is most clear because Immutable- might be interpreted as "the returned collection is immutable" instead of that it will return the same collection every time; and Static- clashes with the reserved word in my opinion
| * @see AsyncTableBuilder#setRequestAttributesFactory(RequestAttributesFactory) | ||
| */ | ||
| @InterfaceAudience.Public | ||
| public interface RequestAttributesFactory { |
There was a problem hiding this comment.
I think it still makes sense to have this factory, mainly for documenting it's behavior; open to replacing with a Supplier<String, byte[]> though
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
3 participants
Summary
Adds
RequestAttributesFactorytoAsyncTableBuilderfor generating request attributes dynamically per-request.Motivation
It is possible to do this for the HBase 2
Tableclient by overriding theRpcControllerFactoryon the connection, but that doesn't work reliably withAsyncTablebecause retries happen on Netty threads, losing thread-local context. This change provides a hook that is guaranteed to be called on the initiating thread.Changes
RequestAttributesFactoryinterface with a singlecreate(Map<String, byte[]>)methodsetRequestAttributesFactory()toAsyncTableBuilderget,put,scan,batch, etc.)getRequestAttributes()returns static attributes; factory is only used for actual requestsUsage