Replace pre publication failed to commit cluster state exceptions #135706

joshua-adams-1 · 2025-09-30T15:03:36Z

This is the third part of a series of PRs fixing how the FailedToCommitClusterStateException is used in ElasticSearch. As per #135017, FailedToCommitClusterStateException is defined as:

Thrown when a cluster state publication fails to commit the new cluster state. If publication fails then a new master is elected but the
update might or might not take effect, depending on whether the newly-elected master accepted the published state that failed to
be committed. This exception should only be used when there is <i>ambiguity</i> whether a state update took effect or not.

Currently, FailedToCommitClusterStateException is used as a 'catch-all' exception thrown at multiple places throughout the Coordinator and MasterService during the publication process. Semantically however, it doesn't make sense to throw this exception before the cluster state update is actually sent over the wire, since at this point, we know for certain that the cluster state update failed. FailedToCommitClusterStateException is intended to display ambiguity.

This work is a pre-requisite to #134213.

Changes

I replace three FailedToCommitClusterStateExceptions thrown prior to publishing the cluster state with NotMasterExceptions as per this conversation with David Turner.

Next Steps

The goal of this work is to fix up all erroneously used FailedToCommitClusterStateException.

Done:

Update exception messages: Update exception messages #135017
Update the FailedToCommitClusterStateException thrown inside MasterService.Batch.onResponse() when draining the queue after the threadpool has shut down - Change FailedToCommitClusterStateException to NotMasterException #135008
Change a FailedToCommitClusterStateException to NotMasterException during the pre-publication process: Changes FailedToCommitClusterStateException to NotMasterException #135548

Todo:

Replace the FailedToCommitClusterStateException exception inside MasterService.BatchingTaskQueue.submitTask, (here) with a NotMasterException.

Relates to: ES-13061

Changes a FailedToCommitClusterStateException incorrectly thrown prior to cluster state update publication to a NotMasterException

- Introduces a FailedToPublishClusterStateException. - Changes three FailedToCommitClusterStateExceptions thrown prior to the cluster state update publication to FailedToPublishClusterStateExceptions. Relates to: ES-13061

joshua-adams-1

Highlighting which changes have been included since I built this PR on top of #135548

joshua-adams-1 · 2025-09-30T15:03:52Z

server/src/main/java/org/elasticsearch/cluster/NotMasterException.java

        super(in);
    }

+    public NotMasterException(String msg, Object... args) {


Changed as part of #135548 and will disappear once I rebase

joshua-adams-1 · 2025-09-30T15:04:02Z

server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java

                        )
                    );
-                    throw new FailedToCommitClusterStateException(
+                    throw new NotMasterException(


Changed as part of #135548 and will disappear when I rebase

JeremyDahlgren · 2025-10-01T13:58:20Z

server/src/main/java/org/elasticsearch/cluster/service/MasterService.java

+                if (e instanceof FailedToCommitClusterStateException) {
+                    failure = new FailedToCommitClusterStateException(e.getMessage(), e);
+                } else {
+                    failure = new NotMasterException(e.getMessage(), e);
+                }


Should we also be handling FailedToPublishClusterStateException here too?

Yes! Good catch - I realised I'm missing code here, and in a few other places too

joshua-adams-1 · 2025-10-02T08:19:25Z

As a note to anyone wanting to review, I need to push a second revision, adding the FailedToPublishClusterStateException to a few more files, and also extending a number of test suites to randomly generate this error during tests to increase the coverage

…exceptions' of https://github.com/joshua-adams-1/elasticsearch into replace-pre-publication-failed-to-commit-cluster-state-exceptions

…ster-state-exceptions

elasticsearchmachine · 2025-10-02T13:10:21Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

DaveCTurner · 2025-10-03T11:16:18Z

...c/main/java/org/elasticsearch/cluster/coordination/FailedToPublishClusterStateException.java

+ * <p>
+ * This is a retryable exception inside {@link TransportMasterNodeAction}
+ */
+public class FailedToPublishClusterStateException extends ElasticsearchException {


Is this meaningfully different from NotMasterException? I know the name isn't ideal, but introducing a new ElasticsearchException subclass carries substantial costs too.

What do you mean by costs? I proposed adding a new exception to make the code easier to understand, especially since NotMasterException implies the error occurs because the node is no longer the master which isn't true in these cases

We also need to think about BwC concerns - what happens if you're in a mixed-version cluster and this exception gets thrown to an older node which doesn't know that it's retryable?

Can you point to a case where NotMasterException doesn't imply that the node has (or will very shortly have) stopped being the master?

We also need to think about BwC concerns - what happens if you're in a mixed-version cluster and this exception gets thrown to an older node which doesn't know that it's retryable?

Would something like this protect against mixed version clusters?

if (getVersion().onOrAfter(TransportVersion.THE_VERSION_I_ADDED_ABOVE)) { throw new FailedToPublishClusterStateException(); } else { throw new FailedToCommitClusterStateException() }

Can you point to a case where NotMasterException doesn't imply that the node has (or will very shortly have) stopped being the master?

My proposed solution changed the three FailedToCommitClusterStateExceptions below into FailedToPublishClusterStateExceptions:

@Override public void publish( ClusterStatePublicationEvent clusterStatePublicationEvent, ActionListener<Void> publishListener, AckListener ackListener ) { try { synchronized (mutex) { if (mode != Mode.LEADER || getCurrentTerm() != clusterStatePublicationEvent.getNewState().term()) { logger.debug( () -> format( "[%s] failed publication as node is no longer master for term %s", clusterStatePublicationEvent.getSummary(), clusterStatePublicationEvent.getNewState().term() ) ); // === Changed in #135548 === // throw new NotMasterException( "node is no longer master for term " + clusterStatePublicationEvent.getNewState().term() + " while handling publication" ); } if (currentPublication.isPresent()) { assert false : "[" + currentPublication.get() + "] in progress, cannot start new publication"; logger.error( () -> format( "[%s] failed publication as already publication in progress", clusterStatePublicationEvent.getSummary() ) ); // === Exception 1 === // throw new FailedToCommitClusterStateException("publication " + currentPublication.get() + " already in progress"); } assert assertPreviousStateConsistency(clusterStatePublicationEvent); final ClusterState clusterState; final long publicationContextConstructionStartMillis; final PublicationTransportHandler.PublicationContext publicationContext; final PublishRequest publishRequest; try { clusterState = clusterStatePublicationEvent.getNewState(); assert getLocalNode().equals(clusterState.getNodes().get(getLocalNode().getId())) : getLocalNode() + " should be in published " + clusterState; publicationContextConstructionStartMillis = transportService.getThreadPool().rawRelativeTimeInMillis(); publicationContext = publicationHandler.newPublicationContext(clusterStatePublicationEvent); } catch (Exception e) { logger.debug(() -> "[" + clusterStatePublicationEvent.getSummary() + "] publishing failed during context creation", e); becomeCandidate("publication context creation"); // === Exception 2 === // throw new FailedToCommitClusterStateException("publishing failed during context creation", e); } try (Releasable ignored = publicationContext::decRef) { try { clusterStatePublicationEvent.setPublicationContextConstructionElapsedMillis( transportService.getThreadPool().rawRelativeTimeInMillis() - publicationContextConstructionStartMillis ); publishRequest = coordinationState.get().handleClientValue(clusterState); } catch (Exception e) { logger.warn( "failed to start publication of state version [" + clusterState.version() + "] in term [" + clusterState.term() + "] for [" + clusterStatePublicationEvent.getSummary() + "]", e ); becomeCandidate("publication creation"); // === Exception 3 === // throw new FailedToCommitClusterStateException("publishing failed while starting", e); } ....

If a publication is already in progress, AFAIU this implies the current node is not the master, because only master nodes can initiate cluster state updates. But what if this is on the new master running at the same time as the old? Can this occur? Because in this case, a NotMasterException would not make sense.

I'm not sure this implies the node will not be the master anymore, since I followed the code through and we can throw an ElasticsearchException here if serialization fails, and that's independent of a node being master.

AFAICT this can be safely converted to a NotMasterException. Digging into the .handleClientValue(clusterState) function I see code throwing exceptions such as:

throw new CoordinationStateRejectedException("election not won"); throw new CoordinationStateRejectedException("cannot start publishing next value before accepting previous one"); throw new CoordinationStateRejectedException( "incoming term " + clusterState.term() + " does not match current term " + getCurrentTerm() ); ...

which all imply the current node is not the master anymore since there are term mismatches, and so a NotMasterException is correct

DaveCTurner · 2025-10-06T11:08:04Z

Would something like this protect against mixed version clusters?

What do you mean by getVersion()? But no not really, we'd still be in a position where we're misusing FailedToCommitClusterStateException when really we mean NotMasterException.

changed the three FailedToCommitClusterStateExceptions below into FailedToPublishClusterStateExceptions:

Case 1 has an assert false - this can't happen in practice. Cases 2 and 3 have a becomeCandidate() call so the node is indeed not the master any more at this point. I think we should use NotMasterException in these cases.

instead

…ster-state-exceptions

DaveCTurner

LGTM (just comment nits but no need for another round)

DaveCTurner · 2025-10-07T13:19:08Z

...rc/main/java/org/elasticsearch/cluster/coordination/FailedToCommitClusterStateException.java

- *
- * This exception is retryable within {@link TransportMasterNodeAction}.
- *
+ * Exception indicating a cluster state update was published but not committed to all nodes.


Conceptual nit: "committed" is a global property rather than something that happens on one or more nodes.

Suggested change

* Exception indicating a cluster state update was published but not committed to all nodes.

* Exception indicating a cluster state update was published and may or may not have been committed.

This exception indicates the publishing master doesn't think the update was committed, but it cannot tell for sure. It depends on which other master nodes accepted it and the winner of the next election.

DaveCTurner · 2025-10-07T13:19:21Z

...rc/main/java/org/elasticsearch/cluster/coordination/FailedToCommitClusterStateException.java

+ * Exception indicating a cluster state update was published but not committed to all nodes.
+ * <p>
+ * If this exception is thrown, then the cluster state update was published, but is not guaranteed
+ * to be committed on any nodes, including the next master node. This exception should only be thrown when there is


Suggested change

* to be committed on any nodes, including the next master node. This exception should only be thrown when there is

* to be committed, including the next master node. This exception should only be thrown when there is

DaveCTurner

Oh yeah also could you adjust the top-level comment in the PR to look more like a commit message? I generally recommend always having this reflect the eventual commit message, and if you need to give more context to reviewers etc then do so in a reply. Otherwise you'll forget to adjust it on merge sometimes and it'll make a mess of the git log.

Also maybe (if you think it's not obvious) should we comment on why NotMasterException is appropriate in these cases because of the preceding becomeCandidate()?

joshua-adams-1 added 2 commits September 26, 2025 16:53

Changes FailedToCommitClusterStateException to NotMasterException

0a0418d

Changes a FailedToCommitClusterStateException incorrectly thrown prior to cluster state update publication to a NotMasterException

Add FailedToPublishClusterStateException

f2c836d

- Introduces a FailedToPublishClusterStateException. - Changes three FailedToCommitClusterStateExceptions thrown prior to the cluster state update publication to FailedToPublishClusterStateExceptions. Relates to: ES-13061

joshua-adams-1 requested review from DiannaHohensee, DaveCTurner and JeremyDahlgren September 30, 2025 15:03

joshua-adams-1 self-assigned this Sep 30, 2025

joshua-adams-1 added >refactoring :Distributed Coordination/Distributed A catch all label for anything in the Distributed Coordination area. Please avoid if you can. labels Sep 30, 2025

elasticsearchmachine added the v9.2.0 label Sep 30, 2025

joshua-adams-1 commented Sep 30, 2025

View reviewed changes

JeremyDahlgren reviewed Oct 1, 2025

View reviewed changes

elasticsearchmachine added v9.3.0 and removed v9.2.0 labels Oct 2, 2025

joshua-adams-1 changed the title ~~Replace pre publication failed to commit cluster state exceptions~~ [WIP] Replace pre publication failed to commit cluster state exceptions Oct 2, 2025

joshua-adams-1 and others added 8 commits October 2, 2025 12:48

Extend use of FailedToPublishClusterStateException

350da55

[CI] Update transport version definitions

eec4c8f

[CI] Auto commit changes from spotless

812ce64

Spotless Apply

8df45c6

Merge branch 'replace-pre-publication-failed-to-commit-cluster-state-…

01ffa86

…exceptions' of https://github.com/joshua-adams-1/elasticsearch into replace-pre-publication-failed-to-commit-cluster-state-exceptions

Trying to get transport definitions to work

4911606

Merge branch 'main' into replace-pre-publication-failed-to-commit-clu…

b76a1e5

…ster-state-exceptions

Finally got transport definitions to work

7376b9d

joshua-adams-1 changed the title ~~[WIP] Replace pre publication failed to commit cluster state exceptions~~ Replace pre publication failed to commit cluster state exceptions Oct 2, 2025

joshua-adams-1 marked this pull request as ready for review October 2, 2025 13:09

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Oct 2, 2025

joshua-adams-1 mentioned this pull request Oct 2, 2025

Remove FailedToCommitClusterStateException Check #135846

Open

joshua-adams-1 requested a review from JeremyDahlgren October 3, 2025 08:59

DaveCTurner reviewed Oct 3, 2025

View reviewed changes

joshua-adams-1 added 4 commits October 7, 2025 11:56

Remove FailedToPublishClusterStateException and uses NotMasterException

4152c42

instead

Merge branch 'main' into replace-pre-publication-failed-to-commit-clu…

0eb27a8

…ster-state-exceptions

Fix merge issues

f886182

Fix merge issues

a277c2e

joshua-adams-1 mentioned this pull request Oct 7, 2025

Replace FailedToCommitClusterStateException with NotMasterException #136083

Open

joshua-adams-1 requested a review from DaveCTurner October 7, 2025 12:18

DaveCTurner approved these changes Oct 7, 2025

View reviewed changes

DaveCTurner reviewed Oct 7, 2025

View reviewed changes

	* Exception indicating a cluster state update was published but not committed to all nodes.
	* Exception indicating a cluster state update was published and may or may not have been committed.

	* to be committed on any nodes, including the next master node. This exception should only be thrown when there is
	* to be committed, including the next master node. This exception should only be thrown when there is

Replace pre publication failed to commit cluster state exceptions #135706

Are you sure you want to change the base?

Replace pre publication failed to commit cluster state exceptions #135706

Conversation

joshua-adams-1 commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Next Steps

Done:

Todo:

Uh oh!

joshua-adams-1 left a comment

Choose a reason for hiding this comment

Uh oh!

joshua-adams-1 Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshua-adams-1 Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshua-adams-1 commented Oct 2, 2025

Uh oh!

elasticsearchmachine commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Oct 6, 2025

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshua-adams-1 commented Sep 30, 2025 •

edited

Loading

joshua-adams-1 Sep 30, 2025 •

edited

Loading

joshua-adams-1 Sep 30, 2025 •

edited

Loading