-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-25329 Dump region hashes in logs for the regions that are stuck in transition for more than a configured amount of time #2761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
💔 -1 overall
This message was automatically generated. |
2 similar comments
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
9978a2f to
59962a4
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice changes, left few comments
| if (oldestRITTime < ritTime) { | ||
| oldestRITTime = ritTime; | ||
| } | ||
| if (counter < 500) { // Record 500 oldest RITs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case cluster is already facing some perf issues, we might want to reduce this count. Let's make this configurable.
| metrics.updateRITCount(ritStat.getTotalRITs()); | ||
| metrics.updateRITCountOverThreshold(ritStat.getTotalRITsOverThreshold()); | ||
| if ((EnvironmentEdgeManager.currentTime() - ritStat.ritThreshold / 2) >= ritStat.statTimestamp){ | ||
| LOG.debug("Oldest RIT hashes and states: "+ritStat.getOldestRITHashesAndStates().toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are anyways exposing rits in metrics, writing DEBUG logs might be overkill I believe. Let's keep the log at TRACE level?
| } | ||
| } | ||
|
|
||
| public final static RegionStateDurationComparator REGION_STATE_DURATION_COMPARATOR = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: keep it private?
|
@virajjasani @bharathv @apurtell updated this PR to dump to logs only. any more suggestions here? if not I will also update PRs for branch-1 and branch-2. thanks! |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understood the patch, can't we just dump 'ritsOverThreshold' at the end of RegionInTransitionStat#update()?
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
We can, if we want only the RITs over threshold logged, and not all the RITs. If that is better I'll update the patch. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
4f757b4 to
1ee1775
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. |
| update(regionStates.getRegionFailedOpen(), statTimestamp); | ||
|
|
||
| if (ritsOverThreshold != null && !ritsOverThreshold.isEmpty()) { | ||
| LOG.trace("RIT hashes and states: " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating logs here looks better. Just small nit: instead of appending string, can we use placeholder {} here?
LOG.trace("RIT hashes and states: {}", val);
… in transition for more than a configured amount of time
1ee1775 to
fcbb912
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nits, lgtm otherwise.
| update(regionStates.getRegionFailedOpen(), statTimestamp); | ||
|
|
||
| if (ritsOverThreshold != null && !ritsOverThreshold.isEmpty()) { | ||
| LOG.trace("RIT hashes and states: {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "RITs over threshold...." ? (otherwise it appears these are all the RITs)
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@bharathv @virajjasani if this PR looks good, can we merge it? |
|
@caroliney14 Yes good to go unless @virajjasani has any more comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
Signed-off-by: Bharath Vissapragada <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> (cherry picked from commit 6d6b8ec) Change-Id: Iffa49f0dece0bf5d241575fb5827d5a41ea0f7ad
No description provided.