HBASE-26807 Unify CallQueueTooBigException special pause with CallDroppedException #4273
Conversation
This file contains 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
|
💔 -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. |
bbeaudreault
force-pushed
the
HBASE-26807-2
branch
2 times, most recently
from
c42ca47 to
56cccb4
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. |
bbeaudreault
force-pushed
the
HBASE-26807-2
branch
from
56cccb4 to
f1eb85f
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. |
bbeaudreault
commented
Apr 7, 2022
| @@ -118,50 +95,45 @@ | |||
| private final int maxKeyValueSize; | |||
|
|
|||
| AsyncConnectionConfiguration(Configuration conf) { | |||
| ConnectionConfiguration connectionConf = new ConnectionConfiguration(conf); | |||
There was a problem hiding this comment.
note this difference between master and branch-2 -- since we must maintain both an async and blocking configuration, I pulled this in here to DRY up the handling (and remove the duplicate logging)
bbeaudreault
commented
Apr 7, 2022
| @@ -119,8 +119,8 @@ public AsyncRpcRetryingCallerFactory(AsyncConnectionImpl conn, Timer retryTimer) | |||
| return this; | |||
| } | |||
|
|
|||
| public SingleRequestCallerBuilder<T> pauseForCQTBE(long pause, TimeUnit unit) { | |||
| this.pauseForCQTBENs = unit.toNanos(pause); | |||
| public SingleRequestCallerBuilder<T> pauseForServerOverloaded(long pause, TimeUnit unit) { | |||
There was a problem hiding this comment.
this class is IA.Private, so I simply rename all of these methods rather than handle deprecation
|
@ndimiduk We should let this branch run the pre-commit checks before merging, but I just checked and it should apply cleanly to branch-2.5 as well. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Ok unfortunately I think the test failure might be real. I was able to reproduce locally, and branch-2 succeeded. I don't have time to look into this right now because I'm headed out of town. I'll be back to look at it on Tuesday, so we might want to hold off merging until I can get to it @ndimiduk |
|
💔 -1 overall
This message was automatically generated. |
ndimiduk
added
the
backport
…for server overloaded to blocking client
bbeaudreault
force-pushed
the
HBASE-26807-2
branch
from
bfb330c to
c3a2a55
Compare
|
The test failure was due to a somewhat implicit dependency between HTableMultiplexer and AsyncProcess conf parsing. I fixed that by creating a new AsyncProcess constuctor for use by HTableMultiplier which upgrades the dependency to explicit. This way we're unlikely to hit issues like this again in the future. The change is covered by existing tests since they failed previously and now succeed. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Your extra commit for branch-2 looks fine to me. Will you need to apply that change as an addendum to |
|
I don't think we need an addendum for master. All of the extra commits here are just to support branch-2 specific functionality (i.e blocking client). HTableMultiplexer doesn't exist in master. The one divergence between master and branch-2 that might matter is the difference in in AsyncConnectionConfiguration, where here we instantiate a |
|
I was thinking of creating a jira to clean up ConnectionConfiguration in master, but not sure how much time I'll have for that in the near future. |
bbeaudreault
added a commit
that referenced
this pull request
Apr 22, 2022
…ppedException (#4273) Signed-off-by: Nick Dimiduk <[email protected]>
bbeaudreault
added a commit
to HubSpot/hbase
that referenced
this pull request
Apr 24, 2022
…pause with CallDroppedException (apache#4273) Signed-off-by: Nick Dimiduk <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is the branch-2 port for #4180
The first commit is a simple cherry-pick of that PR.
The second commit applies the same changes to the blocking RPC client classes. In implementing for blocking client, I consolidated the configuration parsing within ConnectionConfiguration. In my opinion this is much cleaner/DRY, and also mirrors how AsyncConnectionConfiguration works for the async client.
Added tests for each of the 3 places that these pauses end up in the blocking client: AsyncProcess, RpcRetryingCallerImpl, and ConnectionImplementation.locationRegions.