-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Introduce index.version.compatibility setting #83264
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
Changes from all commits
934929a
23ac60a
e20a16f
df45f90
bd3edfc
a5546ab
293b2e9
4026f06
051787a
5ca36cc
01021cd
e26e644
f0d45c6
b8f8aec
776100b
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 |
|---|---|---|
|
|
@@ -336,6 +336,49 @@ public static APIBlock readFrom(StreamInput input) throws IOException { | |
| @Deprecated | ||
| public static final String SETTING_VERSION_UPGRADED_STRING = "index.version.upgraded_string"; | ||
|
|
||
| public static final String SETTING_VERSION_COMPATIBILITY = "index.version.compatibility"; | ||
|
|
||
| /** | ||
| * See {@link #getCompatibilityVersion()} | ||
| */ | ||
| public static final Setting<Version> SETTING_INDEX_VERSION_COMPATIBILITY = Setting.versionSetting( | ||
henningandersen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| SETTING_VERSION_COMPATIBILITY, | ||
| SETTING_INDEX_VERSION_CREATED, // fall back to index.version.created | ||
| new Setting.Validator<>() { | ||
|
|
||
| @Override | ||
| public void validate(final Version compatibilityVersion) { | ||
|
|
||
| } | ||
|
|
||
| @Override | ||
| public void validate(final Version compatibilityVersion, final Map<Setting<?>, Object> settings) { | ||
| Version createdVersion = (Version) settings.get(SETTING_INDEX_VERSION_CREATED); | ||
| if (compatibilityVersion.before(createdVersion)) { | ||
| throw new IllegalArgumentException( | ||
| SETTING_VERSION_COMPATIBILITY | ||
| + " [" | ||
| + compatibilityVersion | ||
| + "] must be >= " | ||
| + SETTING_VERSION_CREATED | ||
| + " [" | ||
| + createdVersion | ||
| + "]" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Iterator<Setting<?>> settings() { | ||
| final List<Setting<?>> settings = List.of(SETTING_INDEX_VERSION_CREATED); | ||
| return settings.iterator(); | ||
| } | ||
|
|
||
| }, | ||
| Property.IndexScope, | ||
| Property.PrivateIndex | ||
| ); | ||
|
|
||
| /** | ||
| * The user provided name for an index. This is the plain string provided by the user when the index was created. | ||
| * It might still contain date math expressions etc. (added in 5.0) | ||
|
|
@@ -484,6 +527,7 @@ public static APIBlock readFrom(StreamInput input) throws IOException { | |
| private final DiscoveryNodeFilters initialRecoveryFilters; | ||
|
|
||
| private final Version indexCreatedVersion; | ||
| private final Version indexCompatibilityVersion; | ||
|
|
||
| private final ActiveShardCount waitForActiveShards; | ||
| private final ImmutableOpenMap<String, RolloverInfo> rolloverInfos; | ||
|
|
@@ -592,6 +636,7 @@ private IndexMetadata( | |
| this.autoExpandReplicas = autoExpandReplicas; | ||
| this.isSearchableSnapshot = isSearchableSnapshot; | ||
| this.isPartialSearchableSnapshot = isPartialSearchableSnapshot; | ||
| this.indexCompatibilityVersion = SETTING_INDEX_VERSION_COMPATIBILITY.get(settings); | ||
| assert numberOfShards * routingFactor == routingNumShards : routingNumShards + " must be a multiple of " + numberOfShards; | ||
| } | ||
|
|
||
|
|
@@ -689,11 +734,23 @@ public long primaryTerm(int shardId) { | |
| /** | ||
| * Return the {@link Version} on which this index has been created. This | ||
| * information is typically useful for backward compatibility. | ||
| * To check index compatibility (e.g. N-1 checks), use {@link #getCompatibilityVersion()} instead. | ||
| */ | ||
| public Version getCreationVersion() { | ||
| return indexCreatedVersion; | ||
| } | ||
|
|
||
| /** | ||
| * Return the {@link Version} that this index provides compatibility for. | ||
| * This is typically compared to the {@link Version#minimumIndexCompatibilityVersion()} to figure out whether the index can be handled | ||
|
Contributor
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. This is likely mostly my uncertainty on the project and intentions. It nags me a little that we instantiate the new mappings/metadata on a specific version and then guarantee to be able to use that on the next major, but no longer than that. Both aspects of that seems wrong and should really be tied to the created-version (if version 10 can read 5 indices, there is no need to do anything about an index restored in 7.x during upgrade to 8, 9 and 10). I suppose we could upgrade metadata/mappings for every major. And also on minors in case we change how we derive the metadata/mapping. Is that part of the intention? It makes me wonder if we could do this completely dynamically (even at the shard level to support mixed clusters), but that also sounds complex.
Contributor
Author
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.
Correct, that's the intention with the compatibility version. Similarly to the created-version, it defines how "old" the index metadata is and what legacy stuff from the past it might contain. At the same time, I envision this to be at least auto-upgraded every major (preserving / remapping as much of the original metadata as possible). This is why again we have the natural N-1 compatibility with Note that as we preserve the original mapping as well (under _meta/legacy-mappings), it might make sense to remap across minors even to make use of some new features (e.g. doc-value-only fields for new field types). |
||
| * by the cluster. | ||
| * By default, this is equal to the {@link #getCreationVersion()}, but can also be a newer version if the index has been imported as | ||
|
Contributor
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. I think we should adapt the javadoc on
Contributor
Author
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. fixed in f0d45c6 |
||
| * a legacy index from an older snapshot, and its metadata has been converted to be handled by newer version nodes. | ||
| */ | ||
| public Version getCompatibilityVersion() { | ||
| return indexCompatibilityVersion; | ||
| } | ||
|
|
||
| public long getCreationDate() { | ||
| return creationDate; | ||
| } | ||
|
|
@@ -1923,11 +1980,12 @@ public static IndexMetadata legacyFromXContent(XContentParser parser) throws IOE | |
| } else if (token == XContentParser.Token.START_OBJECT) { | ||
| if ("settings".equals(currentFieldName)) { | ||
| Settings settings = Settings.fromXContent(parser); | ||
| if (SETTING_INDEX_VERSION_CREATED.get(settings).onOrAfter(Version.CURRENT.minimumIndexCompatibilityVersion())) { | ||
| if (SETTING_INDEX_VERSION_COMPATIBILITY.get(settings) | ||
| .onOrAfter(Version.CURRENT.minimumIndexCompatibilityVersion())) { | ||
| throw new IllegalStateException( | ||
| "this method should only be used to parse older index metadata versions " | ||
| "this method should only be used to parse older incompatible index metadata versions " | ||
| + "but got " | ||
| + SETTING_INDEX_VERSION_CREATED.get(settings) | ||
| + SETTING_INDEX_VERSION_COMPATIBILITY.get(settings) | ||
| ); | ||
| } | ||
| builder.settings(settings); | ||
|
|
@@ -2008,7 +2066,7 @@ public static IndexMetadata legacyFromXContent(XContentParser parser) throws IOE | |
| } | ||
|
|
||
| IndexMetadata indexMetadata = builder.build(); | ||
| assert indexMetadata.getCreationVersion().before(Version.CURRENT.minimumIndexCompatibilityVersion()); | ||
| assert indexMetadata.getCompatibilityVersion().before(Version.CURRENT.minimumIndexCompatibilityVersion()); | ||
| return indexMetadata; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1285,9 +1285,7 @@ public ClusterState execute(ClusterState currentState) { | |
|
|
||
| final ImmutableOpenMap.Builder<ShardId, ShardRestoreStatus> shardsBuilder = ImmutableOpenMap.builder(); | ||
|
|
||
| final Version minIndexCompatibilityVersion = skipVersionChecks(repositoryMetadata) | ||
| ? Version.fromString("1.0.0") | ||
| : currentState.getNodes().getMaxNodeVersion().minimumIndexCompatibilityVersion(); | ||
| final Version minIndexCompatibilityVersion = currentState.getNodes().getMaxNodeVersion().minimumIndexCompatibilityVersion(); | ||
| final String localNodeId = clusterService.state().nodes().getLocalNodeId(); | ||
| for (Map.Entry<String, IndexId> indexEntry : indicesToRestore.entrySet()) { | ||
| final IndexId index = indexEntry.getValue(); | ||
|
|
@@ -1297,10 +1295,9 @@ public ClusterState execute(ClusterState currentState) { | |
| request.indexSettings(), | ||
| request.ignoreIndexSettings() | ||
| ); | ||
| if (snapshotIndexMetadata.getCreationVersion() | ||
| .before(currentState.getNodes().getMaxNodeVersion().minimumIndexCompatibilityVersion())) { | ||
| if (snapshotIndexMetadata.getCompatibilityVersion().before(minIndexCompatibilityVersion)) { | ||
|
Contributor
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. I wonder if we should add an opt-in option to this behavior. I see two ways this could help:
I see that we still have this under a feature flag so no changes needed here, but would be happy to hear your thoughts.
Contributor
Author
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. For the "restore/mount from snapshot" flow, I'm not sure how someone would "inadvertently" restore/mount an old index. They have chosen the index to be restored, so it should be restored (today such a restore would fail, in the future it will go through). It might help if we would show index version information via snapshot status APIs so that users know in advance which indices might be older and which ones not so that they can do a more informed choice on restore. This feels like a problem to be solved on the snapshot discovery APIs (see backlinks to #54940). WDYT? For the major version upgrade of a cluster, there are a number of paths to explore. I'm less focusing right now on the second scenario you described, as that only becomes relevant once we are at the end of 8.x (as there is no 7.x releases of this feature), i.e. the only approach we can recommend in the immediate future is to allow users to take a snapshot in their old cluster, and restore that to a parallel 8.x cluster to try it out. What you're describing will certainly be useful once we go towards the next major. What I'm also thinking more deeply about right now is whether we can find a way to allow users to do a rolling / full cluster restart upgrade from 7.last to 8.x with 6.x indices still present in the cluster that satisfy certain conditions (e.g. warm/cold/frozen tiers) and that will be auto-upgraded to legacy mode, not having to trigger the process via snapshots. Here, we could also require users to take certain steps using existing APIs on 7.x (e.g. flush/mark index read-only) for this auto-upgrade to be successful.
Contributor
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.
One case is to do a restore of all indices. If you restore a 7.x snapshot in an 8.x one, I think we would silently degrade functionality and performance of 6.x indices. My intuition would be to error instead and then ask to add an explicit "archive everything before 7.0" mode in the request.
I think our upgrade assistant would advice to reindex such data. I wonder if we would not need to extend that with this functionality - and thus let it explicitly set a flag to archive on upgrade (with a warning to upgrade to at least 8.some-version)?
Contributor
Author
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.
I'm not sure about the situation where you would encounter this (restore of all indices into next major):
I'm generally wary of having to require an extra flag, as it means that any existing place / UI that allows operating with / restoring snapshots will have to become aware of this flag. Not requiring an extra flag would make this new capability of restoring older indices directly available everywhere. For the in-place upgrade (upgrading cluster to next major version, with some indices in the cluster now becoming legacy indices), I agree that this should not just happen silently.
Contributor
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. OK, thanks for your thoughts on this, I do agree there is a trade-off here. |
||
| // adapt index metadata so that it can be understood by current version | ||
| snapshotIndexMetadata = convertLegacyIndex(snapshotIndexMetadata); | ||
| snapshotIndexMetadata = convertLegacyIndex(snapshotIndexMetadata, currentState); | ||
| } | ||
| try { | ||
| snapshotIndexMetadata = indexMetadataVerifier.verifyIndexMetadata(snapshotIndexMetadata, minIndexCompatibilityVersion); | ||
|
|
@@ -1590,7 +1587,10 @@ public void clusterStateProcessed(ClusterState oldState, ClusterState newState) | |
| } | ||
| } | ||
|
|
||
| private IndexMetadata convertLegacyIndex(IndexMetadata snapshotIndexMetadata) { | ||
| private IndexMetadata convertLegacyIndex(IndexMetadata snapshotIndexMetadata, ClusterState clusterState) { | ||
| if (snapshotIndexMetadata.getCreationVersion().before(Version.fromString("5.0.0"))) { | ||
| throw new IllegalArgumentException("can't restore an index created before version 5.0.0"); | ||
| } | ||
| MappingMetadata mappingMetadata = snapshotIndexMetadata.mapping(); | ||
| Map<String, Object> loadedMappingSource = mappingMetadata.rawSourceAsMap(); | ||
|
|
||
|
|
@@ -1613,7 +1613,17 @@ private IndexMetadata convertLegacyIndex(IndexMetadata snapshotIndexMetadata) { | |
| Map<String, Object> newMapping = new LinkedHashMap<>(); | ||
| newMapping.put(mappingMetadata.type(), newMappingSource); | ||
| // TODO: _routing? Perhaps we don't need to obey any routing here as stuff is read-only anyway and get API will be disabled | ||
| return IndexMetadata.builder(snapshotIndexMetadata).putMapping(new MappingMetadata(mappingMetadata.type(), newMapping)).build(); | ||
| return IndexMetadata.builder(snapshotIndexMetadata) | ||
| .putMapping(new MappingMetadata(mappingMetadata.type(), newMapping)) | ||
| .settings( | ||
| Settings.builder() | ||
| .put(snapshotIndexMetadata.getSettings()) | ||
| .put( | ||
| IndexMetadata.SETTING_INDEX_VERSION_COMPATIBILITY.getKey(), | ||
| clusterState.getNodes().getSmallestNonClientNodeVersion() | ||
|
Contributor
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 use
Contributor
Author
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. I've used the same logic as we use for index creation, as this felt similar in nature. I agree that the interplay with the version barrier is confusing, but it's something to be addressed everywhere then (index creation is probably more frequent than what's being done here).
Contributor
Author
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. I prefer to leave this to a follow-up, as it's a more general change to make (to index creation as well). |
||
| ) | ||
| ) | ||
| .build(); | ||
| } | ||
|
|
||
| private static IndexMetadata.Builder restoreToCreateNewIndex(IndexMetadata snapshotIndexMetadata, String renamedIndexName) { | ||
|
|
||
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.
index.version.createdconcerns both metadata and data. I think this new settings concerns only metadata/mappings and perhaps we should make that explicit in the name?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.
I've been going back and forth on that myself a few times. It also concerns the data, but just a tiny bit (i.e. we "upgraded" the segments during recovery, creating a new commit point). Initially I thought about using a name like
index.version.upgradedto denote that some "upgrade" has taken place, but that name was already taken by some legacy functionality.