-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Cluster local health call to throw exception if node is decommissioned or weighed away #6198
Changes from all commits
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 |
---|---|---|
|
@@ -51,7 +51,10 @@ | |
import org.opensearch.cluster.health.ClusterHealthStatus; | ||
import org.opensearch.cluster.metadata.IndexNameExpressionResolver; | ||
import org.opensearch.cluster.metadata.ProcessClusterEventTimeoutException; | ||
import org.opensearch.cluster.node.DiscoveryNode; | ||
import org.opensearch.cluster.routing.NodeWeighedAwayException; | ||
import org.opensearch.cluster.routing.UnassignedInfo; | ||
import org.opensearch.cluster.routing.WeightedRoutingUtils; | ||
import org.opensearch.cluster.routing.allocation.AllocationService; | ||
import org.opensearch.cluster.service.ClusterService; | ||
import org.opensearch.common.Strings; | ||
|
@@ -140,12 +143,13 @@ protected void clusterManagerOperation( | |
final ClusterState unusedState, | ||
final ActionListener<ClusterHealthResponse> listener | ||
) { | ||
if (request.ensureNodeCommissioned() | ||
if (request.ensureNodeWeighedIn() | ||
&& discovery instanceof Coordinator | ||
&& ((Coordinator) discovery).localNodeCommissioned() == false) { | ||
listener.onFailure(new NodeDecommissionedException("local node is decommissioned")); | ||
return; | ||
} | ||
|
||
final int waitCount = getWaitCount(request); | ||
|
||
if (request.waitForEvents() != null) { | ||
|
@@ -274,7 +278,18 @@ private void executeHealth( | |
|
||
final Predicate<ClusterState> validationPredicate = newState -> validateRequest(request, newState, waitCount); | ||
if (validationPredicate.test(currentState)) { | ||
listener.onResponse(getResponse(request, currentState, waitCount, TimeoutState.OK)); | ||
ClusterHealthResponse clusterHealthResponse = getResponse(request, currentState, waitCount, TimeoutState.OK); | ||
if (request.ensureNodeWeighedIn() && clusterHealthResponse.hasDiscoveredClusterManager()) { | ||
DiscoveryNode localNode = clusterService.state().getNodes().getLocalNode(); | ||
Comment on lines
-277
to
+283
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. Can we assert that local node is true? 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. added 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. Why are we picking up the new state of the cluster and not the one where the observer actually succeeded. IMO, this should be 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. didn't understand this completely. Do you mean |
||
assert request.local() == true : "local node request false for request for local node weighed in"; | ||
boolean weighedAway = WeightedRoutingUtils.isWeighedAway(localNode.getId(), clusterService.state()); | ||
if (weighedAway) { | ||
listener.onFailure(new NodeWeighedAwayException("local node is weighed away")); | ||
return; | ||
} | ||
} | ||
|
||
listener.onResponse(clusterHealthResponse); | ||
} else { | ||
final ClusterStateObserver observer = new ClusterStateObserver( | ||
currentState, | ||
|
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.
Will this cause BWC failures? If yes we can retain both flags and deprecate the former
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
2.6.0
is not released yet, do you this BWC is required? I am looking out for test failures in gradle build, will that be sufficient?