Make TransportLocalClusterStateAction wait for cluster to unblock #117230

nielsbauman · 2024-11-21T07:58:33Z

This will make TransportLocalClusterStateAction wait for a new state
that is not blocked. This means we need a timeout (again). For
consistency's sake, we're reusing the REST param master_timeout for
this timeout as well.

The only class that was using TransportLocalClusterStateAction was
TransportGetAliasesAction, so its request needed to accept a timeout
again as well.

This will make `TransportLocalClusterStateAction` wait for a new state that is not blocked. This means we need a timeout (again). For consistency's sake, we're reusing the REST param `master_timeout` for this timeout as well. The only class that was using `TransportLocalClusterStateAction` was `TransportGetAliasesAction`, so its request needed to accept a timeout again as well.

nielsbauman · 2024-11-21T08:03:17Z

docs/reference/cat/alias.asciidoc


 include::{es-ref-dir}/rest-api/common-parms.asciidoc[tag=expand-wildcards]

+include::{es-ref-dir}/rest-api/common-parms.asciidoc[tag=master-timeout]


Not sure if these docs and rest-api-spec changes are out of scope or not for this PR.

Seems ok to me, tho note that we have never before supported ?master_timeout on these APIs, this isn't reinstating a parameter that previously was removed.

Yeah I realized that, but I figured it made sense to add them if they're parameters we're accepting.

nielsbauman · 2024-11-21T08:04:03Z

server/src/main/java/module-info.java

 exports org.elasticsearch.action.support.master;
 exports org.elasticsearch.action.support.master.info;
 exports org.elasticsearch.action.support.nodes;
+ exports org.elasticsearch.action.support.local;


I moved TransportLocalClusterStateAction to its own local package - together with the new LocalClusterStateRequest.

nielsbauman · 2024-11-21T08:05:10Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesRequest.java

 public GetAliasesRequest(String... aliases) {
- this.aliases = aliases;
- this.originalAliases = aliases;
+ this(MasterNodeRequest.TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, aliases);


It felt out of scope for this PR to refactor all these constructor callers to provide an explicit master timeout - that will have to be done in a follow-up PR.

I would rather we did those changes first to set things up for this PR to go through without having to add back this trappy parameter.

How would we do those changes first if there is currently - on main - no timeout to set in these constructors? Are you suggesting adding a constructor on main that takes a timeout but doesn't actually do anything with it and swapping that constructor with the ones on this branch afterward?

Ah, hm, good point. Ok, let's do it this way round.

server/src/main/java/org/elasticsearch/action/support/local/LocalClusterStateRequest.java

elasticsearchmachine · 2024-11-21T08:07:50Z

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

elasticsearchmachine · 2024-11-21T08:07:50Z

Hi @nielsbauman, I've created a changelog YAML for you.

nielsbauman · 2024-11-21T08:18:21Z

@elasticmachine update branch

server/src/main/java/org/elasticsearch/action/support/local/LocalClusterStateRequest.java

...r/src/main/java/org/elasticsearch/action/support/local/TransportLocalClusterStateAction.java

DaveCTurner · 2024-11-21T08:28:06Z

...r/src/main/java/org/elasticsearch/action/support/local/TransportLocalClusterStateAction.java

+ );
+ listener.onFailure(new ElasticsearchTimeoutException("timed out while waiting for cluster to unblock", exception));
+ }
+ }, clusterState -> isTaskCancelled(task) || checkBlock(request, clusterState) == null);


This means that if the task is cancelled while waiting for an unblocking cluster state then it'll wait for the next cluster state update before completing, which could be arbitrarily far in the future. Could we use org.elasticsearch.tasks.CancellableTask#addListener to react more promptly to cancellation instead?

Good idea 👍. It looks like we're not doing that in TransportMasterNodeAction either. Am I missing something or should we add it there too?

Eh we probably should. But that's a good point, let's leave this with the same behaviour as in TransportMasterNodeAction and we can come back to this change later.

I opened #117971 for this.

DaveCTurner · 2024-11-21T08:29:45Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesRequest.java

 public GetAliasesRequest(String... aliases) {
- this.aliases = aliases;
- this.originalAliases = aliases;
+ this(MasterNodeRequest.TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, aliases);


I would rather we did those changes first to set things up for this PR to go through without having to add back this trappy parameter.

DaveCTurner · 2024-11-21T08:30:38Z

docs/reference/cat/alias.asciidoc


 include::{es-ref-dir}/rest-api/common-parms.asciidoc[tag=expand-wildcards]

+include::{es-ref-dir}/rest-api/common-parms.asciidoc[tag=master-timeout]


Seems ok to me, tho note that we have never before supported ?master_timeout on these APIs, this isn't reinstating a parameter that previously was removed.

DaveCTurner

Looks good, just a few superficial things

DaveCTurner · 2024-12-02T08:55:33Z

docs/changelog/117230.yaml

@@ -0,0 +1,5 @@
+pr: 117230
+summary: Make `TransportLocalClusterStateAction` wait for cluster to unblock


I think we could describe this change in a more user-facing way: we're fixing that the various aliases APIs don't wait for a block to be removed. The fact that we're doing this by changing TransportLocalClusterStateAction is not really relevant to the release notes.

I changed the summary to Make various alias retrieval APIs wait for cluster to unblock. Let me know if you have any suggestions and/or if you think we should elaborate with a description.

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesRequest.java

server/src/main/java/org/elasticsearch/action/support/local/LocalClusterStateRequest.java

.../test/java/org/elasticsearch/action/support/local/TransportLocalClusterStateActionTests.java

DaveCTurner · 2024-12-02T09:06:56Z

.../test/java/org/elasticsearch/action/support/local/TransportLocalClusterStateActionTests.java

+
+ public void testRetryAfterBlock() throws ExecutionException, InterruptedException {
+ var request = new Request();
+ ClusterBlock block = new ClusterBlock(1, "", true, true, false, randomFrom(RestStatus.values()), ClusterBlockLevel.ALL);


nit: cluster block ID 1 is special - it means the STATE_NOT_RECOVERED_BLOCK. Could we randomize this to indicate that the ID doesn't matter to these tests.

I replaced it with randomInt(). I saw some places using randomInt() and some with a lower and upper bound. Let me know if you think bounding the random int is preferable.

.../test/java/org/elasticsearch/action/support/local/TransportLocalClusterStateActionTests.java

DaveCTurner

LGTM

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.0.0 labels Nov 21, 2024

nielsbauman force-pushed the local-transport-action branch from e6c4cfd to b4f4cf5 Compare November 21, 2024 08:02

nielsbauman commented Nov 21, 2024

View reviewed changes

nielsbauman requested a review from DaveCTurner November 21, 2024 08:07

elasticsearchmachine added the Team:Distributed Indexing Meta label for Distributed Indexing team label Nov 21, 2024

elasticsearchmachine removed the Team:Distributed Coordination Meta label for Distributed Coordination team label Nov 21, 2024

Update docs/changelog/117230.yaml

725436d

Merge branch 'main' into local-transport-action

42b2f89

DaveCTurner reviewed Nov 21, 2024

View reviewed changes

nielsbauman added 3 commits November 21, 2024 12:19

Merge branch 'main' into local-transport-action

9a378b4

Rename request field to masterTimeout

6fc9f81

Use else instead of early return

a5199fe

nielsbauman requested a review from DaveCTurner November 21, 2024 12:04

DaveCTurner reviewed Dec 2, 2024

View reviewed changes

nielsbauman added 2 commits December 2, 2024 14:48

Merge branch 'main' into local-transport-action

dd0d486

PR feedback

ae2fa02

nielsbauman requested a review from DaveCTurner December 2, 2024 14:36

DaveCTurner approved these changes Dec 4, 2024

View reviewed changes

nielsbauman merged commit 032b42f into elastic:main Dec 4, 2024
16 checks passed

nielsbauman deleted the local-transport-action branch December 4, 2024 11:17

nielsbauman mentioned this pull request Dec 4, 2024

React more promptly to task cancellation while waiting for the cluster to unblock #117971

Closed

pquentin mentioned this pull request Jul 22, 2025

[8.19] Remove unsupported master_timeout from cat.aliases API elastic/elasticsearch-specification#5007

Merged


		include::{es-ref-dir}/rest-api/common-parms.asciidoc[tag=expand-wildcards]

		include::{es-ref-dir}/rest-api/common-parms.asciidoc[tag=master-timeout]

		@@ -0,0 +1,5 @@
		pr: 117230
		summary: Make `TransportLocalClusterStateAction` wait for cluster to unblock

Make TransportLocalClusterStateAction wait for cluster to unblock #117230

Make TransportLocalClusterStateAction wait for cluster to unblock #117230

Uh oh!

Conversation

nielsbauman commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Nov 21, 2024

elasticsearchmachine commented Nov 21, 2024

nielsbauman commented Nov 21, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Labels

4 participants