Desired nodes API #82975
Desired nodes API #82975
Conversation
fcofdez
force-pushed
the
desired-nodes-api
branch
from
562370d
to
42097f2
Compare
fcofdez
added
Team:Distributed
| "processors" : 8, | ||
| "memory" : "58gb", | ||
| "storage" : "1700gb", | ||
| "node_version" : "8.1.0" |
There was a problem hiding this comment.
I think there isn't a way to fetch this value programatically and populate it, I think we should upgrade it as new versions come out. Maybe it's better to skip the doc tests?
There was a problem hiding this comment.
I think it is ok to leave that for a follow-up at least, i.e.,m skip the doc tests in this PR.
| listener.onResponse(new GetDesiredNodesAction.Response(DesiredNodesMetadata.latestFromClusterState(state))); | ||
| final DesiredNodes latestDesiredNodes = DesiredNodesMetadata.latestFromClusterState(state); | ||
| if (latestDesiredNodes == null) { | ||
| throw new ResourceNotFoundException("Desired nodes not found"); |
There was a problem hiding this comment.
I prefer to invoke listener.onFailure directly rather than throwing exceptions even though it will need an else block below.
| final DesiredNodes latestDesiredNodes = DesiredNodesMetadata.latestFromClusterState(newState); | ||
| boolean replacedExistingHistoryId = previousDesiredNodes != null | ||
| && previousDesiredNodes.hasSameHistoryId(latestDesiredNodes) == false; | ||
| listener.onResponse(new UpdateDesiredNodesResponse(true, replacedExistingHistoryId)); |
There was a problem hiding this comment.
I think we should simply remove the acknowledged flag from the response, just like adding voting config exclusions do not have it. The client should not care about acknowledged, only whether data is committed or not (which should throw if not or in doubt).
|
|
||
| @Override | ||
| public void clusterStateProcessed(ClusterState oldState, ClusterState newState) { | ||
| final DesiredNodes previousDesiredNodes = DesiredNodesMetadata.latestFromClusterState(oldState); |
There was a problem hiding this comment.
Maybe add comment that we rely on the unbatched executor here?
| ) { | ||
| super( | ||
| UpdateDesiredNodesAction.NAME, | ||
| transportService, |
There was a problem hiding this comment.
I think we should allow this even if above circuit breaker limit, this is orchestration trying to help us out:
| transportService, | |
| false, | |
| transportService, |
|
|
||
| clusterService.submitStateUpdateTask( | ||
| "update-desired-nodes", | ||
| new ClusterStateUpdateTask(Priority.HIGH, request.masterNodeTimeout()) { |
There was a problem hiding this comment.
I would think we should use URGENT here instead, but maybe @DaveCTurner has an opinion on that?
There was a problem hiding this comment.
I'd be ok with URGENT although I'd be happier about it if these things were batched, just in case the orchestrator goes haywire.
There was a problem hiding this comment.
I'll update the PR batching them
| } | ||
|
|
||
| if (nodes.isEmpty()) { | ||
| validationException = ValidateActions.addValidationError("nodes must contain at least one node", validationException); |
There was a problem hiding this comment.
In a follow-up we should probably also verify that there is at least one master eligible node.
| @@ -166,7 +172,7 @@ public void testUnknownSettingsAreAllowedInFutureVersions() { | |||
| assertThat(latestDesiredNodes, is(equalTo(desiredNodes))); | |||
| } | |||
|
|
|||
| public void testSomeSettingsCanBeOverridden() { | |||
| public void testNodeProcessorsGetValidatedWithDesiredNodeProcessors() { | |||
There was a problem hiding this comment.
I think this verifies what the method name says by not throwing when setting the desired nodes? Perhaps add a comment if so.
| ClusterState state, | ||
| ActionListener<AcknowledgedResponse> listener | ||
| ) throws Exception { | ||
| clusterService.submitStateUpdateTask("delete-desired-nodes", new AckedClusterStateUpdateTask(Priority.HIGH, request, listener) { |
There was a problem hiding this comment.
I think these don't need to be acked tasks, we don't care whether the update is applied on all nodes or not since it will only be used on the master. It's enough that the state update is committed.
I'd rather we didn't merge without addressing this comment.
|
Thanks all for the reviews! |
Add the dry_run query parameter to support simulating of updating of desired nodes. The update request will be validated, but no cluster state updates will be performed. In order to indicate that the response was a result of a dry run, we add the dry_run run field to the JSON representation of a response. See #82975
This commit adds the Desired Nodes API, allowing orchestrators
that manage Elasticsearch clusters to let the system know about the
current/planned topology that the cluster will run on.
This allows the system to take better decisions based on the entire
cluster topology, including nodes that will be added/removed in the
near future.
This commit adds the basic endpoints to manage the desired nodes
state: