-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Make sure explanation is populated when .not_preferred logging is turned on #138325
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
Changes from 2 commits
2372936
00f2e43
45702c4
c9a8751
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -862,17 +862,14 @@ public boolean moveShards() { | |||||||||||
| for (var storedShardMovement : bestNonPreferredShardMovementsTracker.getBestShardMovements()) { | ||||||||||||
| final var shardRouting = storedShardMovement.shardRouting(); | ||||||||||||
| final var index = projectIndex(shardRouting); | ||||||||||||
| // If `shardMoved` is true, there may have been moves that have made our previous move decision | ||||||||||||
| // invalid, so we must call `decideMove` again. If not, we know we haven't made any moves, and we | ||||||||||||
| // can use the cached decision. | ||||||||||||
| final var moveDecision = shardMoved ? decideMove(index, shardRouting) : storedShardMovement.moveDecision(); | ||||||||||||
| final var moveDecision = reassessDecisionIfRequired(index, storedShardMovement, shardMoved); | ||||||||||||
| if (moveDecision.isDecisionTaken() && moveDecision.cannotRemainAndCanMove()) { | ||||||||||||
| if (notPreferredLogger.isDebugEnabled()) { | ||||||||||||
| notPreferredLogger.debug( | ||||||||||||
| "Moving shard [{}] to [{}] from a NOT_PREFERRED allocation, explanation is [{}]", | ||||||||||||
| "Moving shard [{}] to [{}] from a NOT_PREFERRED allocation: {}", | ||||||||||||
| shardRouting, | ||||||||||||
| moveDecision.getTargetNode().getName(), | ||||||||||||
| moveDecision.getCanRemainDecision().getExplanation() | ||||||||||||
| moveDecision.getCanRemainDecision() | ||||||||||||
| ); | ||||||||||||
| } | ||||||||||||
| executeMove(shardRouting, index, moveDecision, "move-non-preferred"); | ||||||||||||
|
|
@@ -886,6 +883,36 @@ public boolean moveShards() { | |||||||||||
| return shardMoved; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * If a shard has moved we need to re-run the decision to check that it's still valid. If {@link #notPreferredLogger} is | ||||||||||||
| * set to debug, we re-run the decision with explain turned on so we can populate the explanation in our log message | ||||||||||||
| * | ||||||||||||
| * @param index The index of the shard | ||||||||||||
| * @param storedShardMovement The existing shard movement decision | ||||||||||||
| * @param shardMoved True if a shard moved in this balancing round, false otherwise | ||||||||||||
| * @return The move decision to act on | ||||||||||||
|
||||||||||||
| */ | ||||||||||||
| private MoveDecision reassessDecisionIfRequired( | ||||||||||||
| ProjectIndex index, | ||||||||||||
| BestShardMovementsTracker.StoredShardMovement storedShardMovement, | ||||||||||||
| boolean shardMoved | ||||||||||||
| ) { | ||||||||||||
| final boolean needToReassess = notPreferredLogger.isDebugEnabled() || shardMoved; | ||||||||||||
| if (needToReassess == false) { | ||||||||||||
nicktindall marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||
| return storedShardMovement.moveDecision(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| final var oldDebugMode = allocation.getDebugMode(); | ||||||||||||
| if (notPreferredLogger.isDebugEnabled()) { | ||||||||||||
| allocation.setDebugMode(RoutingAllocation.DebugMode.EXCLUDE_YES_DECISIONS); | ||||||||||||
| } | ||||||||||||
|
Comment on lines
+916
to
+918
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left this one because I don't think either form is significantly more or less readable, and it seems weird to set the value if it's not being changed. |
||||||||||||
| try { | ||||||||||||
| return decideMove(index, storedShardMovement.shardRouting()); | ||||||||||||
| } finally { | ||||||||||||
| allocation.setDebugMode(oldDebugMode); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private void executeMove(ShardRouting shardRouting, ProjectIndex index, MoveDecision moveDecision, String reason) { | ||||||||||||
| final ModelNode sourceNode = nodes.get(shardRouting.currentNodeId()); | ||||||||||||
| final ModelNode targetNode = nodes.get(moveDecision.getTargetNode().getId()); | ||||||||||||
|
|
||||||||||||
Uh oh!
There was an error while loading. Please reload this page.