HBASE-27657: Connection and Request Attributes #5306
Closed
Conversation
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
rmdmattingly
force-pushed
the
HBASE-27657-custom-rpc-controller
branch
4 times, most recently
from
dbad8a8 to
eceef19
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
HBASE-27657-custom-rpc-controller
branch
from
eceef19 to
86f29f9
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Jun 23, 2023
There was a problem hiding this comment.
Thanks for picking this up and preparing the PR.
Left a few nits, not very critial.
My concern here is how our users could make use of this feature. Users can send customized connection headers or request headers at client side, but how do they plan to make use of these attributes at server side?
Better post a design doc to describe the more about the new feature especially the usage, and also, let's include a test or an example in hbase-examples module to show how to make use of this feature.
Thanks.
|
|
||
| @Override | ||
| public Map<String, byte[]> getAttributes() { | ||
| return null; |
There was a problem hiding this comment.
Good point/question — this is done in a sense because the default controller won't provide any request attributes. I could think about whether there's a way to make the interface simpler here, so that rather than needing to provide a custom controller a user could just set some RequestHeaderProvider in the Configuration that's then called here. But I think that might be a little tricky and would add some significant amount of code that still, in its default form, has nothing to provide
| */ | ||
| public static Connection createConnection(Configuration conf, ExecutorService pool, | ||
| final User user) throws IOException { | ||
| return createConnection(conf, pool, user, null); |
There was a problem hiding this comment.
Better just use Collections.emptyMap here, so we do not need a null check in later code, usually.
| */ | ||
| package org.apache.hadoop.hbase.ipc; | ||
|
|
||
| import static org.apache.hadoop.hbase.shaded.protobuf.generated.RPCProtos.*; |
Good question. Right now there isn't much out-of-the-box that one will get server-side via these attributes. One could trivially begin using these attributes in their coprocessor(s), for example. Medium/long term, we also have plans to use these attributes at my day job by implementing a new type of quota. At my company we run thousands of microservices, some of which are very fundamental and consequently have a variety of "upstream callers". These proxy APIs, from HBase's perspective, are a single user and can only be throttled as such — but we may have some single pathologic upstream caller that ideally could be throttled in isolation. Another common, similar, problem we see is MapReduce jobs accidentally creating hotspots. Our solution is to support custom quota groupings (HBASE-27784) which could be powered by, for example, request attributes. From the issue:
Also 👍 on the design doc / hbase-examples. If we have agreement on the vague shape of things here then I'll definitely go ahead on building out examples! |
|
Since These could be used in various ways in coprocessors depending on the implementation. They also exist as a building block for other native features in the future, like Quota Groups that Ray mentioned. We also plan to expose them in SlowLog payload, so one could get valuable tracing information from their clients. Sounds like Duo did not have an issue with the API defined here, so we could just encode this into a simple design doc with slightly more formal version of the above to describe how they could be used and why they are an improvement over Operation attributes. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
HBASE-27657-custom-rpc-controller
branch
from
e6b079a to
17b5159
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
I think this test failure is unrelated/noise. Can we re-run? |
|
I've replied on the jira issue as I do not have the permission to comment on the design doc... PTAL. Thanks. |
|
Thank you for taking a look! I’m on vacation this week so I will think about the feedback and get back to you early next week |
|
For posterity, we've continued the conversation here: https://issues.apache.org/jira/browse/HBASE-27657?focusedCommentId=17741604&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17741604 |
|
After the aforementioned discussion, we're going to rethink the approach here a bit so I'm going to close this PR and reopen when we have a more straightforward/user-friendly implementation. Thanks for the feedback here! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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
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#createConnectionWe've also added a test which tests both systems end-to-end.