HBASE-25329 Dump region hashes in logs for the regions that are stuck in transition for more than a configured amount of time #2762
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
caroliney14
force-pushed
the
HBASE-25329-branch-1
branch
2 times, most recently
from
67198af to
61c1278
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
d-c-manning
reviewed
Dec 12, 2020
| if (oldestRITTime < ritTime) { | ||
| oldestRITTime = ritTime; | ||
| } | ||
| if (counter < 500) { // Record 500 oldest RITs |
There was a problem hiding this comment.
500 seems high to me. Would 20 be enough? Should this be a constant in the file, even if not configurable?
|
|
||
| LOG.debug("Oldest RIT hashes and states: " + oldestRITHashesAndStates.toString()); | ||
| long time = EnvironmentEdgeManager.currentTime(); | ||
| if ((time - ritThreshold / 2) >= this.lastRITHashMetricUpdate) { |
There was a problem hiding this comment.
Why do we update the metrics hashes only if it's been more than ritThreshold / 2 time since last update? If we do the work to find the oldest, it seems like we should update the metrics always. Then any query to get metrics will always have the most recent results (~3 seconds old at max, with default hbase.regionserver.msginterval)
Though I do think it's a good idea for limiting the logging. Should the LOG.debug statement be in here?
Then this statement may deserve a comment.
Suggested change
| if ((time - ritThreshold / 2) >= this.lastRITHashMetricUpdate) { | |
| // Only log if it has been long enough since the last update (default 30 seconds) | |
| if ((time - ritThreshold / 2) >= this.lastRITHashMetricUpdate) { |
There was a problem hiding this comment.
Though I do think it's a good idea for limiting the logging. Should the LOG.debug statement be in here?
yes it is for the reason you stated. will add the comment and move LOG.debug under this condition. maybe the other stuff can be moved outside.
| this.metricsAssignmentManager.updateRITCountOverThreshold(totalRITsOverThreshold); | ||
|
|
||
| LOG.debug("Oldest RIT hashes and states: " + oldestRITHashesAndStates.toString()); | ||
| long time = EnvironmentEdgeManager.currentTime(); |
There was a problem hiding this comment.
can we just reuse currentTime here from the beginning of the method?
| getInt(HConstants.METRICS_RIT_STUCK_WARNING_THRESHOLD, 60000); | ||
| for (RegionState state: regionStates.getRegionsInTransition()) { | ||
| int counter = 0; | ||
| for (RegionState state: regionStates.getRegionsInTransitionOrderedByDuration()) { |
There was a problem hiding this comment.
since this method uses the state.getStamp() to determine whether the RIT is over the threshold or the oldest time... should we just use getRegionsInTransitionOrderedByTimestamp instead?
This new method getRegionsInTransitionOrderedByDuration is tracking total duration instead of duration since last state transition, so the longest RITs may not necessarily be those that are reported over threshold. I think it's probably good to be consistent... I'm not sure which way is better to be consistent, but since these metrics in this method are reporting time since last change instead of total duration, I'd favor that approach.
| oldestRITTime = ritTime; | ||
| } | ||
| if (counter < 500) { // Record 500 oldest RITs | ||
| oldestRITHashesAndStates.add( |
There was a problem hiding this comment.
should we also filter these to only those RITs that are over the threshold? Or do we want to display every RIT, even if it's only been transitioning for 100ms, if it's the longest/only RIT in the cluster?
There was a problem hiding this comment.
I think it makes sense to only record RITs over threshold, since those would be the problematic ones
| } | ||
| if (counter < 500) { // Record 500 oldest RITs | ||
| oldestRITHashesAndStates.add( | ||
| state.getRegion().getRegionNameAsString() + ":" + state.getState().name() |
There was a problem hiding this comment.
Do we have to use name() as I see other references use state.getState() directly.
region names could be quite long - when you refer to hash, did you want the md5 encoded name?
Suggested change
| state.getRegion().getRegionNameAsString() + ":" + state.getState().name() | |
| state.getRegion().getEncodedName() + ":" + state.getState() |
Or should the entire region name be available?
There was a problem hiding this comment.
if the md5 encoded name is enough info for debugging (which I think yes), then it should suffice to use that 👍
There was a problem hiding this comment.
as for state, that is an enum, and we want the state name, which is why I added .name(). see this SO
| this.metricsAssignmentManager.updateRITCount(totalRITs); | ||
| this.metricsAssignmentManager.updateRITCountOverThreshold(totalRITsOverThreshold); | ||
|
|
||
| LOG.debug("Oldest RIT hashes and states: " + oldestRITHashesAndStates.toString()); |
There was a problem hiding this comment.
we should only log if oldestRITHashesAndStates is non-empty.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@virajjasani @bharathv do you mind taking a look at this? thanks! |
|
Thanks for putting this together. Definitely useful to have more insights into RIT states. I added a comment in the jira. Would like to know your thoughts. |
|
I just realized that this branch-1 backport PR also has review comments. |
virajjasani
added
the
backport
virajjasani
reviewed
Jan 13, 2021
| if (ritTime > ritThreshold) { // more than the threshold | ||
| totalRITsOverThreshold++; | ||
| if (ritsOverThreshold == null) { | ||
| ritsOverThreshold = new HashMap<String, RegionState>(); |
| if (LOG.isDebugEnabled() && ritsOverThreshold != null && !ritsOverThreshold.isEmpty()) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (String regionName: ritsOverThreshold.keySet()) { | ||
| sb.append(regionName + ":" + ritsOverThreshold.get(regionName).getState().name() + "\n"); |
There was a problem hiding this comment.
nit: use chain of append() to cover String concatenation?
sb.append(regionName).append(":")
.append(ritsOverThreshold.get(regionName).getState().name()).append("\n");
| for (String regionName: ritsOverThreshold.keySet()) { | ||
| sb.append(regionName + ":" + ritsOverThreshold.get(regionName).getState().name() + "\n"); | ||
| } | ||
| sb.delete(sb.length()-2, sb.length()); |
There was a problem hiding this comment.
Do we want to remove last appended new line char? If so, start index should be sb.length()-1 right?
virajjasani
reviewed
Jan 13, 2021
| int totalRITs = 0; | ||
| int totalRITsOverThreshold = 0; | ||
| long oldestRITTime = 0; | ||
| HashMap<String, RegionState> ritsOverThreshold = null; |
There was a problem hiding this comment.
nit: Map<String, RegionState> ritsOverThreshold = null
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
approved these changes
Jan 15, 2021
virajjasani
reviewed
Jan 15, 2021
| } | ||
| if (LOG.isDebugEnabled() && ritsOverThreshold != null && !ritsOverThreshold.isEmpty()) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (String regionName: ritsOverThreshold.keySet()) { |
There was a problem hiding this comment.
If you could convert this to entrySet(), that would be great (as per findbugs)
caroliney14
force-pushed
the
HBASE-25329-branch-1
branch
from
3d173cb to
36e0655
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
reviewed
Jan 20, 2021
Comment on lines
3561
to
3562
| sb.append(regionName).append(":") | ||
| .append(ritsOverThreshold.get(regionName).getState().name()).append("\n"); |
There was a problem hiding this comment.
This should be:
sb.append(regionName.getKey()).append(":")
.append(regionName.getValue().getState().name()).append("\n");
… in transition for more than a configured amount of time
caroliney14
force-pushed
the
HBASE-25329-branch-1
branch
from
111c16f to
295f851
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
No description provided.