Uh oh!
There was an error while loading. Please reload this page.
HBASE-27853 Add client side table metrics for rpc calls and request latency. - #5228
Merged
Merged
Conversation
Apache-HBase
commented
May 10, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 10, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 10, 2023
💔 -1 overall
This message was automatically generated. |
bbeaudreault
requested changes
Aug 11, 2023
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
Sorry it took so long to get eyes on this. Thanks for the submission. I had a few requests
| boolean closeScanner) throws IOException { | ||
| ScanRequest.Builder builder = ScanRequest.newBuilder(); | ||
| RegionSpecifier region = buildRegionSpecifier(RegionSpecifierType.REGION_NAME, regionName); | ||
| builder.setRegion(buildRegionSpecifier(RegionSpecifierType.REGION_NAME, regionName)); |
Contributor
There was a problem hiding this comment.
what is the reason for these changes in this class?
ContributorAuthor
There was a problem hiding this comment.
because the way I obtain the table name is through the region name. However, for a scan query, the region name will only be passed in the first request, and subsequent requests will no longer pass the region name but the scannerId. Therefore, this class needs to be modified, otherwise the table metrics for the scan request cannot be obtained.
Contributor
There was a problem hiding this comment.
Thanks for explaining.
I'm not sure we should modify the RPC protocol just for the sake of these client side metrics. It might be better to use HBaseRpcController to shuttle the table name into the onCallFinished method (where updateRpc is called). As an example, you could modify AsyncScanSingleRegionRpcRetryingCaller.call method:
- resetController(controller, callTimeoutNs, priority);+ resetController(controller, callTimeoutNs, priority, loc.getRegion().getTable());
ScanRequest req = RequestConverter.buildScanRequest(scannerId, scan.getCaching(), false,
nextCallSeq, scan.isScanMetricsEnabled(), false, scan.getLimit());
final Context context = Context.current();
stub.scan(controller, req, resp -> {
try (Scope ignored = context.makeCurrent()) {
onComplete(controller, resp);
}
});This requires a bit more changes, but at least it keeps our RPC protocol unchanged.
You'll need to add a setTableName and getTableName to HBaseRpcController and DelegatingHBaseRpcController. Then update that ConnectionUtils.resetController method (and callers), as well as RegionServerCallable once on branch-2.
@Apache9 if you have time, do you agree with this advice?
ContributorAuthor
There was a problem hiding this comment.
@Apache9@bbeaudreault hi, could you provide me with more assistance so that I can complete this PR? Thank you very much.
Contributor
There was a problem hiding this comment.
Is there something else you’d need? I’d go down the path I described, which I feel pretty good about. Just duo often has nice ideas so I figured I’d ask, but he’s probably busy.
ContributorAuthor
There was a problem hiding this comment.
@bbeaudreault Thank you for your reply! Okay, I will make modifications as you described.
ContributorAuthor
There was a problem hiding this comment.
@bbeaudreault hi, I have made the modifications as per your request. Could you please review the code and provide some suggestions? I would greatly appreciate it!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Aug 13, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 13, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 13, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 20, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 20, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 20, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 20, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 20, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 20, 2023
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
commented
Aug 26, 2023
Contributor
Thanks for making those changes. There are some warnings in the pre commit hooks (checkstyle, etc). Can you fix them? I'm going to be out of office until sept 5, so probably won't be able to re-review this until then. Just fyi, I'll get back to it soon. |
Apache-HBase
commented
Aug 27, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 27, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 27, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 27, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 27, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 27, 2023
🎊 +1 overall
This message was automatically generated. |
zhuyaogai
commented
Sep 8, 2023
ContributorAuthor
@bbeaudreault hi, could you continue the code review for me if you are available? Thanks! |
bbeaudreault
requested changes
Sep 8, 2023
| } | ||
| CompletableFuture<OpenScannerResponse> future = new CompletableFuture<>(); | ||
| try { | ||
| controller.setTableName(loc.getRegion().getTable()); |
Contributor
There was a problem hiding this comment.
I'm pretty sure this is unnecessary. This callOpenScanner ends up getting called via AsyncSingleRequestRpcRetryingCaller, which does resetCallTimeout() before calling the callable. So the change you made in that method should handle this case
| callTimeoutNs = rpcTimeoutNs; | ||
| } | ||
| resetController(controller, callTimeoutNs, priority); | ||
| if (getTableName().isPresent()) { |
Contributor
There was a problem hiding this comment.
could be simplified to one call, with getTableName().orElse(null)
| if (methodName != null) { | ||
| String table; | ||
| if (tableName == null || StringUtils.isEmpty(tableName.getNameAsString())) { | ||
| // Fallback to get table name from region specifier. |
Contributor
There was a problem hiding this comment.
i think we can simplify this whole method (and above calls) now. I'm not sure we need to handle fallback, since all cases should have a TableName in the controller now. So we don't need to extract the region in updateRpc above, nor do we have to to parse the tablename from the region here.
if we missed a spot, i think we'd consider that a bug and fix it. maybe you could add end-to-end tests to ensure that each of the request types updates a table metric?
ContributorAuthor
There was a problem hiding this comment.
@bbeaudreault hi, I have already modified the code according to your requirements. Could you continue the code review for me if you are available? Thanks!
Apache-HBase
commented
Sep 9, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 9, 2023
💔 -1 overall
This message was automatically generated. |
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
This is looking good! One more request.
Also, can you please squash and rebase this PR so that we can clear out the patch warning in the pre-commit hook?
| "Get".equals(method.getName()) || "Mutate".equals(method.getName()) | ||
| || "Scan".equals(method.getName()) || "Multi".equals(method.getName()) | ||
| ) { | ||
| updateTableMetric(methodName.toString(), tableName, stats, e); |
Contributor
There was a problem hiding this comment.
do we still need this, or can we simply add the call to updateTableMetric in the appropriate switch state cases below? Just to avoid unnecessary complexity and equality checks
zhuyaogaiforce-pushed
the
client_table_level_metrics
branch
2 times, most recently
from
September 11, 2023 16:30
5280f8c to
c9dbf05Comparezhuyaogai
commented
Sep 11, 2023
ContributorAuthor
done. |
Apache-HBase
commented
Sep 11, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 11, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 11, 2023
💔 -1 overall
This message was automatically generated. |
bbeaudreault
commented
Sep 11, 2023
Contributor
@zhuyaogai unit test failures look related |
Apache-HBase
commented
Sep 11, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 12, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 12, 2023
💔 -1 overall
This message was automatically generated. |
zhuyaogai
commented
Sep 12, 2023
ContributorAuthor
@bbeaudreault hi, I found that the latest unit test failures seem to be unrelated to me? |
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
You're right, the test failures I was referring to are outdated. The new ones look ok.
I just caught one possible bug, and we can merge after that.
Uh oh!
There was an error while loading. Please reload this page.
zhuyaogaiforce-pushed
the
client_table_level_metrics
branch
from
September 12, 2023 13:28
c9dbf05 to
d186215CompareApache-HBase
commented
Sep 12, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 12, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 12, 2023
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
approved these changes
Sep 12, 2023
bbeaudreault
commented
Sep 12, 2023
Contributor
Test failure looks unrelated. LGTM |
Contributor
@zhuyaogai thanks for all the work here! Can you submit a backport PR for branch-2? I think you will need to add a bit more code for the sync client (HTable) there |
bbeaudreault pushed a commit
that referenced
this pull request
Sep 12, 2023
…atency. (#5228) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
zhuyaogai
commented
Sep 13, 2023
ContributorAuthor
@bbeaudreault Okay, but it might need to be later. By the way, which branch are you referring to when you say branch-2? |
bbeaudreault
commented
Sep 13, 2023
Contributor
Lowercase |
wchevreuil pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Nov 2, 2023
…atency. (apache#5228) 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.
Details: HBASE-27853