Skip to content

Conversation

@williamrandolph
Copy link
Contributor

Our system index management classes need to access minimum system index mappings version information for the cluster as a whole. There are two reasons for this. First, when creating a system index, we may need to retrieve mappings that are compatible with earlier nodes. Second, and more commonly, we need to know when the minimum system index descriptor version has incremented across all nodes, so that we can update mappings. Currently, we use Version for this, but this will move us most of the way towards being able to use mappings versions.

This PR adds the condition to the node join executor that a joining node can't have system index mappings whose version is lower than the minimum version for those mappings on the cluster. In other words, no system index's mapping version can move backwards.

@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but I'd like to combine this with TransportVersion up-front and ideally do it in a few steps for easier reviews. See inline comments.

Comment on lines 186 to +189
private final Map<String, TransportVersion> transportVersions;
private final TransportVersion minTransportVersion;

private final Map<String, VersionsWrapper> versionsWrappers;
private final VersionsWrapper minVersions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we first generalize this per-node TransportVersion map into a VersionsWrapper map (containing just the TransportVersion) and then add the system index version to VersionsWrapper in a separate step? I believe that first step should be possible without changing the transport protocol.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does sound much more reviewable. I'll take a shot at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a refactoring PR that just wraps TransportVersion with no changes to the transport protocol: #99114

// infer the versions from discoverynodes for now
builder.nodes().getNodes().values().forEach(n -> builder.putTransportVersion(n.getId(), inferTransportVersion(n)));
}
if (in.getTransportVersion().onOrAfter(TransportVersion.current())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd strongly recommend not using TransportVersion.current() for this, even in a draft PR - it'd be all too easy to miss one when finalizing things, and CI wouldn't be able to tell you about it until the next transport version bump (by which point the bad version may be released).

williamrandolph added a commit that referenced this pull request Sep 6, 2023
Cluster state currently holds a cluster minimum transport version and a map of nodes to transport versions. However, to determine node compatibility, we will need to account for more types of versions in cluster state than just the transport version (see #99076). Here we introduce a wrapper class to cluster state and update accessors and builders to use the new method. (I would have liked to re-use org.elasticsearch.cluster.node.VersionInformation, but that one holds IndexVersion rather than TransportVersion.

* Introduce CompatibilityVersions to cluster state class
@williamrandolph
Copy link
Contributor Author

We did this work in a series of more reviewable PRs, leading to #99307

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >enhancement v8.11.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants