HBASE-27798: Client side should back off based on wait interval in RpcThrottlingException #5226
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
commented
May 8, 2023
Comment on lines
161
to
167
| // If, after the planned sleep, there won't be enough time left, we stop now. | ||
| long duration = singleCallDuration(expectedSleep); | ||
| if (duration > callTimeout) { | ||
| String msg = "callTimeout=" + callTimeout + ", callDuration=" + duration + ": " | ||
| + t.getMessage() + " " + callable.getExceptionMessageAdditionalDetail(); | ||
| throw (SocketTimeoutException) new SocketTimeoutException(msg).initCause(t); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Moving this check inside of the else block is pretty consequential, and I'm not sure which way we'd prefer.
If we leave this logic outside of the if (t instanceof RTE) else block then significant wait intervals — those which would extend past the call timeout — may be totally ignored. We'd instead just throw quickly which would probably manifest as retries, and consequently sort of produce the opposite of a back off.
Alternatively, by isolating this logic to only non-RTE exceptions, we're more likely to respect the back off requested — even if it will inevitably result in the timeout being exceeded.
Maybe we want to calculate the min of the requested wait interval and the duration remaining prior to timeout, and somewhat guarantee that we wait for that? I'm open to other suggestions too
Contributor
There was a problem hiding this comment.
So this method here is where we do retries. The fact that we're throwing a SocketTimeoutException means that the retries will stop.
I think we should move this logic outside the if. Let's say the wait interval is 10s, but operation timeout is 5s. Honoring the wait interval will cause us to exceed the timeout, which is not good. At the very least we should do a Math.min, but we've also been told by the server that the quota won't be available until 10s. So we know that if we retry in 5s (due to a Math.min), we will fail. So with that said, we should just move the logic outside the if so that we throw an exception if the wait interval extends beyond operation timeout.
Regarding the quota -- if the server says 10s, i dont think it will ever be ready in, say, 8s. The wait interval is calculated based on the the rate of quota refill, so it should be accurate as a lower threshold. What we can't be sure of is if the quota will actually be ready in 10s (may be longer). If there were only 1 caller, the wait interval would be a great indicator of exactly when to retry. But in the case of N > 1 callers, those other callers may also be consuming the quota and the client may retry in 10s but it's still been saturated in the interim and wasn't able to refill enough to serve the request.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
reviewed
May 9, 2023
Contributor
bbeaudreault
left a comment
bbeaudreault
left a comment
There was a problem hiding this comment.
I do think we want to add some tests for this. See TestAsyncClientPauseForServerOverloaded.java as an example where we can create a mock server which throws an RpcThrottlingException. We can have it throw such an exception with a large enough wait interval and set the hbase.client.pause to 0 or something, so that way it's clear that if the method takes longer than a few seconds then it honored our wait interval.
You could also consider extracting this "if throttle do this, otherwise do this" logic into a separate class and just testing that directly. Or you could try keeping the test just on the client side. For the async client, you could create a new test class which extends AsyncRpcRetryingCaller and the doCall just throws an throttle exception. For the non-async client you could just test RpcRetryingCallerImpl directly and pass a callable into callWithRetries which throws a throttle exception.
In terms of master branch, we do need to port this there. It should be relatively simple -- the async client is pretty similar between the two. You'd just exclude the changes to RpcRetryingCallerImpl, since that class doesn't exist in master.
| boolean isServerOverloaded = false; | ||
| if (error instanceof RpcThrottlingException) { | ||
| RpcThrottlingException rpcThrottlingException = (RpcThrottlingException) error; | ||
| pauseNsToUse = rpcThrottlingException.getWaitInterval() * 1000; // wait interval is in millis |
Contributor
There was a problem hiding this comment.
can you use TimeUnit to convert? Then you prob dont need the comment
Comment on lines
161
to
167
| // If, after the planned sleep, there won't be enough time left, we stop now. | ||
| long duration = singleCallDuration(expectedSleep); | ||
| if (duration > callTimeout) { | ||
| String msg = "callTimeout=" + callTimeout + ", callDuration=" + duration + ": " | ||
| + t.getMessage() + " " + callable.getExceptionMessageAdditionalDetail(); | ||
| throw (SocketTimeoutException) new SocketTimeoutException(msg).initCause(t); | ||
| } |
Contributor
There was a problem hiding this comment.
So this method here is where we do retries. The fact that we're throwing a SocketTimeoutException means that the retries will stop.
I think we should move this logic outside the if. Let's say the wait interval is 10s, but operation timeout is 5s. Honoring the wait interval will cause us to exceed the timeout, which is not good. At the very least we should do a Math.min, but we've also been told by the server that the quota won't be available until 10s. So we know that if we retry in 5s (due to a Math.min), we will fail. So with that said, we should just move the logic outside the if so that we throw an exception if the wait interval extends beyond operation timeout.
Regarding the quota -- if the server says 10s, i dont think it will ever be ready in, say, 8s. The wait interval is calculated based on the the rate of quota refill, so it should be accurate as a lower threshold. What we can't be sure of is if the quota will actually be ready in 10s (may be longer). If there were only 1 caller, the wait interval would be a great indicator of exactly when to retry. But in the case of N > 1 callers, those other callers may also be consuming the quota and the client may retry in 10s but it's still been saturated in the interim and wasn't able to refill enough to serve the request.
...ent/src/main/java/org/apache/hadoop/hbase/client/AsyncScanSingleRegionRpcRetryingCaller.java
Show resolved
Hide resolved
rmdmattingly
force-pushed
the
HBASE-27798-branch-2
branch
from
f40a6f6 to
cac69f0
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
HBASE-27798-branch-2
branch
from
cac69f0 to
77efdc8
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
HBASE-27798-branch-2
branch
from
77efdc8 to
6c864e6
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. |
rmdmattingly
force-pushed
the
HBASE-27798-branch-2
branch
from
6c864e6 to
6066db1
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
I'd be surprised if my changes were causing the unit tests to fail to run 🤔 |
rmdmattingly
commented
May 22, 2023
Comment on lines
34
to
37
| long pauseMsForServerOverloaded = TimeUnit.NANOSECONDS.toMillis(pauseNsForServerOverloaded); | ||
| long basePauseMs = TimeUnit.NANOSECONDS.toMillis(pauseNs); | ||
| long pauseMillis = getPauseMillis(t, null, pauseMsForServerOverloaded, basePauseMs); | ||
| return TimeUnit.MILLISECONDS.toNanos(pauseMillis); |
Contributor
Author
There was a problem hiding this comment.
I can definitely see an argument for some duplicative logic rather than these superfluous conversions. happy to make that change if people think it'd be preferable/meaningful
Comment on lines
433
to
435
| long remainingTimeNs = remainingTimeNs(); | ||
| long maxDelayNs = remainingTimeNs - SLEEP_DELTA_NS; | ||
| if (maxDelayNs <= 0 || maxDelayNs <= pauseNsToUse) { |
Contributor
There was a problem hiding this comment.
trying to parse this change... i feel like we can ditch this diff, since we handle the completeExceptionally for throttling above? That was the only change I wanted here, not sure if this is accomplishing something else.
Contributor
Author
There was a problem hiding this comment.
Yeah you're right
rmdmattingly
force-pushed
the
HBASE-27798-branch-2
branch
from
6066db1 to
99a5708
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. |
added 6 commits
May 31, 2023 15:31
rmdmattingly
force-pushed
the
HBASE-27798-branch-2
branch
from
72b2c49 to
707b90d
Compare
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
HBASE-27798-branch-2
branch
from
707b90d to
e2c9c1f
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
reviewed
Jun 1, 2023
Contributor
bbeaudreault
left a comment
bbeaudreault
left a comment
There was a problem hiding this comment.
Looking really good, just a couple more thoughts (really). As you are using the new manager now, it could effectively be static. I was thinking you'd pass in some of the configs in the constructor, but I don't have a strong opinion. As it stands now it might be lighter weight to simple put in ConnectionUtils rather than create a new class with a single static-like method.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncBatchRpcRetryingCaller.java
Show resolved
Hide resolved
| public class HBaseServerExceptionPauseManager { | ||
| private static final Logger LOG = LoggerFactory.getLogger(HBaseServerExceptionPauseManager.class); | ||
|
|
||
| private HBaseServerExceptionPauseManager() {} |
Contributor
There was a problem hiding this comment.
You could pass the Optional in here. You could also pass in the pauseNs values if you want, since they don't change. But not a huge deal
Contributor
Author
There was a problem hiding this comment.
which optional are you referring to? I'm envisioning something like:
private final long pauseNs;
private final long pauseNsForServerOverloaded;
public HBaseServerExceptionPauseManager(long pauseNs, long pauseNsForServerOverloaded) {
this.pauseNs = pauseNs;
this.pauseNsForServerOverloaded = pauseNsForServerOverloaded;
}
public OptionalLong getPauseNsFromException(Throwable error, long remainingTimeNs) {
long expectedSleepNs;
if (error instanceof RpcThrottlingException) {
RpcThrottlingException rpcThrottlingException = (RpcThrottlingException) error;
expectedSleepNs = TimeUnit.MILLISECONDS.toNanos(rpcThrottlingException.getWaitInterval());
if (expectedSleepNs > remainingTimeNs) {
return OptionalLong.empty();
}
if (LOG.isDebugEnabled()) {
LOG.debug("Sleeping for {}ms after catching RpcThrottlingException", expectedSleepNs,
rpcThrottlingException);
}
} else {
expectedSleepNs =
HBaseServerException.isServerOverloaded(error) ? pauseNsForServerOverloaded : pauseNs;
}
return OptionalLong.of(expectedSleepNs);
}
Contributor
There was a problem hiding this comment.
In one of my other most recent comments I suggested doing the metrics incrementing for server overloaded in here so we don't have to handle server overloaded in two places. Not sure if that works, but if so the metrics instance could get passed in here
Contributor
Author
There was a problem hiding this comment.
Oh yeah, I see what you mean! That would be nice. I'm going to omit it here because it would require a somewhat larger diff; the metric reported is the delayNs rather than just the pauseNs, there's some nuance in whether the metric gets reported in the fast failure cases, etc. so I think it would complicate the changeset
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
HBASE-27798-branch-2
branch
from
a261cfa to
28f77ff
Compare
rmdmattingly
force-pushed
the
HBASE-27798-branch-2
branch
from
28f77ff to
ac668cb
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
approved these changes
Jun 5, 2023
rmdmattingly
added a commit
to HubSpot/hbase
that referenced
this pull request
Jun 6, 2023
…cThrottlingException (apache#5226) Signed-off-by: Bryan Beaudreault <[email protected]>
rmdmattingly
added a commit
to HubSpot/hbase
that referenced
this pull request
Jun 8, 2023
…cThrottlingException (apache#5226) Signed-off-by: Bryan Beaudreault <[email protected]>
bbeaudreault
pushed a commit
to HubSpot/hbase
that referenced
this pull request
Jul 12, 2023
…it interval in RpcThrottlingException (apache#5226) Signed-off-by: Bryan Beaudreault <[email protected]>
Sign up for free
to 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.
The RpcThrottlingException tells the client how much to back off, but right now the recommendation is ignored. This PR introduces logic that respects said back off recommendation.
I've tested the synchronous client implementation on a cluster at my day job. Here is the log output from a single client thread that's being throttled by a strict quota: https://gist.github.com/rmdmattingly/4c8e63bec16cfa2e4324ff6590a5f8ea. It demonstrates that the thread is backing off for the given wait interval.
I'm very open to adding more explicit unit testing of this behavior, and/or gating behind some new Configuration option, and would appreciate any suggestions regarding where said tests/config should live.
This also didn't apply particularly cleanly to the master branch, but I'm particularly interested in getting this feature on branch-2. How should I approach this? I'm open to figuring out an equivalent solution against master.
@bbeaudreault