Skip to content
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.

Sign up for GitHub

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

Skip unnecessary string format in RemoteStoreMigrationAllocationDecider when not in debug mode #16299

Merged
merged 2 commits into from
Oct 14, 2024
Merged

Skip unnecessary string format in RemoteStoreMigrationAllocationDecider when not in debug mode #16299

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60c167c
Skip unnecessary string format in RemoteStoreMigrationAllocationDecid…
imRishN Oct 13, 2024
13627a0
Fix
imRishN Oct 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 35 additions & 10 deletions .../opensearch/cluster/routing/allocation/decider/RemoteStoreMigrationAllocationDecider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@
return allocation.decision(
Decision.YES,
NAME,
getDecisionDetails(true, shardRouting, targetNode, " for strict compatibility mode")
getDecisionDetails(true, shardRouting, targetNode, " for strict compatibility mode", allocation)
);
}

Expand All @@ -103,19 +103,23 @@
return allocation.decision(
isNoDecision ? Decision.NO : Decision.YES,
NAME,
getDecisionDetails(!isNoDecision, shardRouting, targetNode, reason)
getDecisionDetails(!isNoDecision, shardRouting, targetNode, reason, allocation)
);
} else if (migrationDirection.equals(Direction.DOCREP)) {
// docrep migration direction is currently not supported
return allocation.decision(Decision.YES, NAME, getDecisionDetails(true, shardRouting, targetNode, " for DOCREP direction"));
return allocation.decision(

Check warning on line 110 in server/src/main/java/org/opensearch/cluster/routing/allocation/decider/RemoteStoreMigrationAllocationDecider.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/cluster/routing/allocation/decider/RemoteStoreMigrationAllocationDecider.java#L110 

Added line #L110 was not covered by tests
Decision.YES,
NAME,
getDecisionDetails(true, shardRouting, targetNode, " for DOCREP direction", allocation)

Check warning on line 113 in server/src/main/java/org/opensearch/cluster/routing/allocation/decider/RemoteStoreMigrationAllocationDecider.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/cluster/routing/allocation/decider/RemoteStoreMigrationAllocationDecider.java#L113 

Added line #L113 was not covered by tests
);
} else {
// check for remote store backed indices
if (remoteSettingsBackedIndex && targetNode.isRemoteStoreNode() == false) {
// allocations and relocations must be to a remote node
String reason = new StringBuilder(" because a remote store backed index's shard copy can only be ").append(
(shardRouting.assignedToNode() == false) ? "allocated" : "relocated"
).append(" to a remote node").toString();
return allocation.decision(Decision.NO, NAME, getDecisionDetails(false, shardRouting, targetNode, reason));
return allocation.decision(Decision.NO, NAME, getDecisionDetails(false, shardRouting, targetNode, reason, allocation));
}

if (shardRouting.primary()) {
Expand All @@ -128,9 +132,9 @@
// handle scenarios for allocation of a new shard's primary copy
private Decision primaryShardDecision(ShardRouting primaryShardRouting, DiscoveryNode targetNode, RoutingAllocation allocation) {
if (targetNode.isRemoteStoreNode() == false) {
return allocation.decision(Decision.NO, NAME, getDecisionDetails(false, primaryShardRouting, targetNode, ""));
return allocation.decision(Decision.NO, NAME, getDecisionDetails(false, primaryShardRouting, targetNode, "", allocation));
}
return allocation.decision(Decision.YES, NAME, getDecisionDetails(true, primaryShardRouting, targetNode, ""));
return allocation.decision(Decision.YES, NAME, getDecisionDetails(true, primaryShardRouting, targetNode, "", allocation));
}

// Checks if primary shard is on a remote node.
Expand All @@ -150,20 +154,41 @@
return allocation.decision(
Decision.NO,
NAME,
getDecisionDetails(false, replicaShardRouting, targetNode, " since primary shard copy is not yet migrated to remote")
getDecisionDetails(
false,
replicaShardRouting,
targetNode,
" since primary shard copy is not yet migrated to remote",
allocation
)
);
}
return allocation.decision(
Decision.YES,
NAME,
getDecisionDetails(true, replicaShardRouting, targetNode, " since primary shard copy has been migrated to remote")
getDecisionDetails(
true,
replicaShardRouting,
targetNode,
" since primary shard copy has been migrated to remote",
allocation
)
);
}
return allocation.decision(Decision.YES, NAME, getDecisionDetails(true, replicaShardRouting, targetNode, ""));
return allocation.decision(Decision.YES, NAME, getDecisionDetails(true, replicaShardRouting, targetNode, "", allocation));
}

// get detailed reason for the decision
private String getDecisionDetails(boolean isYes, ShardRouting shardRouting, DiscoveryNode targetNode, String reason) {
private String getDecisionDetails(
boolean isYes,
ShardRouting shardRouting,
DiscoveryNode targetNode,
String reason,
RoutingAllocation allocation
) {
if (allocation.debugDecision() == false) {
gbbafna marked this conversation as resolved.
Show resolved Hide resolved
return null;
}
return new StringBuilder("[").append(migrationDirection.direction)
.append(" migration_direction]: ")
.append(shardRouting.primary() ? "primary" : "replica")
Expand Down
Loading