-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-28001: Add request attribute support to BufferedMutator #6076
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections from me. This looks like a straightforward extension of our existing feature. It's been a while since I've thought about these classes, but the use-case and API changes look fine to me.
Since this is public API, let's see if others have an opinion here.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncBufferedMutator.java
Outdated
Show resolved
Hide resolved
/** | ||
* Set a rpc request attribute. | ||
*/ | ||
AsyncBufferedMutatorBuilder setRequestAttribute(String key, byte[] value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, or maybe you're hamstrung by the apis that TableBuilder exposes?
Yeah, currently the AsyncBufferedMutatorBuilderImpl
wraps an AsyncTableBuilder
(code).
Maybe we should look at expanding the scope of these methods in a separate JIRA.
I think this is a good idea. When I drafted this PR, I had a difficult time understanding what semantics we support and why we chose them.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncTableImpl.java
Show resolved
Hide resolved
...-server/src/test/java/org/apache/hadoop/hbase/client/TestRequestAndConnectionAttributes.java
Outdated
Show resolved
Hide resolved
Do we have anything that demonstrates use of connection or request attributes in hbase-examples? If yes, please add example overage there. If not, would you mind filing an issue to add something to that module? Thanks @eab148 ! |
We do not. I can create an issue to add request attribute example coverage to the hbase-examples module. |
Makes sense! Happy to field any questions/concerns and make adjustments. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test failure is unrelated to this change
|
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
…#6076) Co-authored-by: Evie Boland <eboland@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Thanks a lot for the contribution @eab148 ! Can you please also provide a new PR for branch-2? You can start by cherry-picking the commit that landed on master. Do ping me when you have the PR up, thanks! |
Co-authored-by: Evie Boland <eboland@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Evie Boland <eboland@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
…#6076) Co-authored-by: Evie Boland <eboland@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Evie Boland <eboland@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
…edMutator (apache#6076) Co-authored-by: Evie Boland <eboland@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
…redMutator (apache#6076) Co-authored-by: Evie Boland <eboland@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
…edMutator (apache#6076) (#116) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Evie Boland <eboland@hubspot.com>
…redMutator (apache#6076) (#118) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Evie Boland <eboland@hubspot.com>
…apache#6076)" This reverts commit 7d019d9.
…#6076) (addendum) Add default implementations of the new methods so that a custom implementation of AsyncBufferedMutatorBuilder will not fail to compile after upgrade.
…#6076) (addendum) Add default implementations of the new methods so that a custom implementation of AsyncBufferedMutatorBuilder will not fail to compile after upgrade.
… (addendum) (#6349) Add default implementations of the new methods so that a custom implementation of AsyncBufferedMutatorBuilder will not fail to compile after upgrade. Signed-off-by: Nihal Jain <nihaljain@apache.org> Signed-off-by: Istvan Toth <stoty@apache.org>
…#6076) (addendum) (apache#6349) Add default implementations of the new methods so that a custom implementation of AsyncBufferedMutatorBuilder will not fail to compile after upgrade. Signed-off-by: Nihal Jain <nihaljain@apache.org> Signed-off-by: Istvan Toth <stoty@apache.org>
… (addendum) (#6349) Add default implementations of the new methods so that a custom implementation of AsyncBufferedMutatorBuilder will not fail to compile after upgrade. Signed-off-by: Nihal Jain <nihaljain@apache.org> Signed-off-by: Istvan Toth <stoty@apache.org>
Design Doc
As of #5326, we have been able to set request attributes on
Table
instances viaTableBuilder::setRequestAttribute
. With request attributes, users can send an attribute once per request, instead of once per operation.In this PR, we extend the feature to BufferedMutators. One can set a request attribute on a BufferedMutator via a new method
BufferedMutatorParams::setRequestAttribute(String key, byte[] value)
.Jira
cc @bbeaudreault @hgromer @bozzkar @rmdmattingly @krconv @sidkhillon