Uh oh!
There was an error while loading. Please reload this page.
HBASE-27657: Connection and Request Attributes - #5326
Merged
bbeaudreault merged 12 commits intoJul 24, 2023
Conversation
rmdmattinglyforce-pushed
the
HBASE-27657-custom-rpc-controller
branch
from
July 18, 2023 14:39
4e771a4 to
ee3eef8Comparermdmattingly
marked this pull request as ready for review
July 18, 2023 14:39
Apache-HBase
commented
Jul 18, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 18, 2023
💔 -1 overall
This message was automatically generated. |
rmdmattingly
commented
Jul 18, 2023
ContributorAuthor
I'll carve out some time to dig into the 👆 failures today |
Apache-HBase
commented
Jul 18, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 18, 2023
💔 -1 overall
This message was automatically generated. |
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
looks pretty straightforward. I had a few small comments.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bbeaudreault
commented
Jul 18, 2023
Contributor
Also, can you fix the spotbugs warning? |
Apache-HBase
commented
Jul 18, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 18, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 18, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 19, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 19, 2023
🎊 +1 overall
This message was automatically generated. |
| /** | ||
| * Sets the map of request attributes | ||
| */ | ||
| AsyncTableBuilder<C> setRequestAttributes(Map<String, byte[]> requestAttributes); |
Contributor
There was a problem hiding this comment.
What do you think about setRequestAttribute(String, byte[]) so that one can more fluently create their Table/AsyncTable if desired? One can chain together ImmutableMap.builder().add().add().build, but not everyone might have it and it's a bit more verbose.
Contributor
There was a problem hiding this comment.
We prob could leave all the *Caller classes as you have them since they are internal and post-build, but for the TableBuilder/AsyncTableBuilder it might be nice.
ContributorAuthor
There was a problem hiding this comment.
Yeah I'd be happy to add that. Just to clarify, we should probably support both, right? And calling setRA after addRA would implicitly overwrite the individual additions?
Contributor
There was a problem hiding this comment.
I don't have a strong opinion on whether we should continue to support setRequestAttributes (with an s). Do you think that has a particular value as is?
If we do support both, then yea we need to figure out the interplay. I agree that calling set after add would overwrite, but what about add after set? One could imagine someone calling set with an immutable map, so we'd probably have to copy it in order to ensure add operations work.
ContributorAuthor
There was a problem hiding this comment.
One could imagine someone calling set with an immutable map, so we'd probably have to copy it in order to ensure add operations work.
Yeah that's a good point.
I don't have a strong opinion on whether we should continue to support setRequestAttributes (with an s). Do you think that has a particular value as is?
I don't feel particularly strongly, but it seems plausible that someone will have a map of requests pre-constructed, particularly if they have several attributes to pass, and that it would be nicer to pass it in than need to iterate calls to addRA
ContributorAuthor
There was a problem hiding this comment.
If we provide a setRequestAttribute(String, byte[]) method then do we also want to support unsetting a key?
ContributorAuthor
There was a problem hiding this comment.
Just pushed a change which allows for the two methods setRequestAttribute and setRequestAttributes. The plural will implicitly overwrite anything already submitted. It also enforces the implementation of map that we're working with
Contributor
There was a problem hiding this comment.
it may be worth simplifying this to all just be additive. I was thinking about it and this is just a temporary builder. To one of your earlier comments, I don't think we need to support removal of attributes and here it's hard to imagine the case for someone to do something like:
getTableBuilder(table)
.setRequestAttribute("foo", "bar")
.setRequestAttribute("abc", "def")
.setRequestAttributes(Map.of("foo", "asdfa")) // actually forget those, i want to just use this
.build()I could see needing to support replacement/removal/etc if we allowed modifying the longer-lived Table objects. I know some builders out there have like a BuiltObject.toBuilder() method, in which case maybe it'd be good to support these things for evolving Tables over time. But our builders are pretty simple and one-off, so not necessary.
So trying to think about KISS here, and to me that means simply additive. I could even imagine changing the plural method to be named setAllRequestAttributes so its maybe a little clearer (along with javadoc) that we're just iterating and calling set on each. If anything, I could imagine a more useful bit of complexity to be supporting typed set methods, like setRequestAttribute(String, String) which does the byte[] conversion for you, along with other primitives. But we can skip that for now :)
One other note -- many people will not use request attributes. Let's default the requestAttributes map to null, and only instantiate a hashmap if one of these setters is called.
Contributor
There was a problem hiding this comment.
Also, I think you need to update the ThriftConnection to override the new method.
ContributorAuthor
There was a problem hiding this comment.
👍 good points, I'll update this morning
Apache-HBase
commented
Jul 19, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2023
💔 -1 overall
This message was automatically generated. |
rmdmattingly
commented
Jul 20, 2023
ContributorAuthor
The test failure seems unrelated |
bbeaudreault
approved these changes
Jul 24, 2023
bbeaudreault pushed a commit
that referenced
this pull request
Jul 24, 2023
Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here's an initial design doc
I'm hoping to get feedback on the design proposed above.
Currently we have the ability to set Operation attributes, via Get.setAttribute, etc. It would be useful to be able to set attributes at the request and connection level.
These levels can result in less duplication. For example, send some attributes once per connection instead of for every one of the millions of requests a connection might send. Or send once for the request, instead of duplicating on every operation in a multi request.
Additionally, the Connection and RequestHeader are more globally available on the server side. Both can be accessed via RpcServer.getCurrentCall(), which is useful in various integration points – coprocessors, custom queues, quotas, slow log, etc. Operation attributes are harder to access because you need to parse the raw Message into the appropriate type to get access to the getter.
This PR introduces two new avenues for providing attributes:
ConnectionFactory#createConnectionTableBuilder#setRequestAttributeWe've also added end-to-end tests for both systems.
cc @bbeaudreault@hgromer@saijmo@eab148@bozzkar