HBASE-29626: Refactor server side scan metrics for Coproc hooks #7340
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
…orrect and cleaner consumption in Coproc hooks
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
virajjasani
changed the title
virajjasani
reviewed
Sep 24, 2025
| } | ||
|
|
||
| @Override | ||
| public long getFsReadTime() { |
There was a problem hiding this comment.
Let's remove getFsReadTime() entirely? It's of no use anyways right?
There was a problem hiding this comment.
Actually I wanted remove these from RpcCall interface but I saw that RpcCall is marked LimitedPrivate with visibility to Phoenix and Coprocs in general while ServerCall its HBase implementation is marked Private.
There was a problem hiding this comment.
Since this is recently introduced, we can remove it for IA.LP
Comment on lines
-151
to
-156
| boolean isScanMetricsEnabled = scan.isScanMetricsEnabled(); | ||
| ThreadLocalServerSideScanMetrics.setScanMetricsEnabled(isScanMetricsEnabled); | ||
| if (isScanMetricsEnabled) { | ||
| this.scannerInitMetrics = new ServerSideScanMetrics(); | ||
| ThreadLocalServerSideScanMetrics.reset(); | ||
| } |
There was a problem hiding this comment.
I am bit confused, do we really not need this at all?
There was a problem hiding this comment.
The removed code in RegionScannerImpl was doing two things:
- Capturing server side scan metrics even when RegionScanner is initialized and passing it to the
next()call inRegionScannerImplto updateServerSideScanMetricsfromScannerContext. - Resetting the state of thread local variable to start fresh for a scan.
For the first point given the call to RegionScannerImpl constructor and next() happens from same thread and its the same thread which is handling RSRpcServices#scan() call so, we can avoid passing around ServerSideScanMetrics from constructor to next() call if we populate ServerSideScanMetrics in RSRpcServices as done for some other scan metrics already.
For second point, I am taking care of resetting the thread local variables for each call in RSRpcServices#scan() call and its much cleaner also.
So, we are still doing same thing in RSRpcServices which we used to do in RegionScannerImpl it's just that its more concise and cleaner now. Please let me know if I am missing something. Thanks
virajjasani
approved these changes
Sep 25, 2025
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
pushed a commit
that referenced
this pull request
Oct 3, 2025
Signed-off-by: Viraj Jasani <[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.
JIRA: HBASE-29626