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
Ensure index.version.created
is consistent
#6660
Conversation
@@ -319,7 +319,7 @@ public ClusterState execute(ClusterState currentState) throws Exception { | |||
} | |||
|
|||
if (indexSettingsBuilder.get(SETTING_VERSION_CREATED) == null) { | |||
indexSettingsBuilder.put(SETTING_VERSION_CREATED, version); | |||
indexSettingsBuilder.put(SETTING_VERSION_CREATED, clusterService.state().nodes().smallestDataNodeVersion()); |
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.
is it true that only data nodes matter? I think all nodes might need to parse the mapping?
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.
@jpountz no we only parse the mapping if we allocate the index on that node. BUT I think we have to take the master into account too. so yeah I am going to fix this...
LGTM except for the comment on using the currentState var |
pushed a new commit |
ImmutableOpenMap<String,Settings> indexToSettings = getSettingsResponse.getIndexToSettings(); | ||
for (int i = 0; i < index.length; i++) { | ||
Settings settings = indexToSettings.get(index[i]); | ||
assertThat(settings.getAsVersion(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT), equalTo(version)); |
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.
Do we want to supply a default value here? If we're adding it for when the master is on 1.2.0 - do we want to verify that version
is before V1_3_0?
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.
we do that already I don't understand you comment sorry :)
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.
Discussed via chat - we should not use the Version.CURRENT as a default to make sure the version is set. The part about a 1.2.0 master is not relevant as it will set the SETTING_VERSION_CREATED key as well.
and another commit now really using the |
LGTM |
LGTM, left one minor comment. |
@bleskes pushed a new commit |
+1 |
Today `index.version.created` depends on the version of the master node in the cluster. This is potentially causing new features to be expected on shards that didn't exist when the index was created. There is no notion of `where was the shard allocated first` such that `index.version.created` can't be reliably used as a feature flag. With this change the `index.version.created` can be reliably used to determin the smallest nodes version at the point in time when the index was created. This means we can safely use certain features that would for instance require reindeing and / or would not work if not the entire index (all shards and segments) have been created with a certain version or newer. Closes elastic#6660
Today `index.version.created` depends on the version of the master node in the cluster. This is potentially causing new features to be expected on shards that didn't exist when the index was created. There is no notion of `where was the shard allocated first` such that `index.version.created` can't be reliably used as a feature flag. With this change the `index.version.created` can be reliably used to determin the smallest nodes version at the point in time when the index was created. This means we can safely use certain features that would for instance require reindeing and / or would not work if not the entire index (all shards and segments) have been created with a certain version or newer. Closes #6660
index.version.created
is consistentindex.version.created
is consistent
index.version.created
is consistentindex.version.created
is consistent
Today
index.version.created
depends on the version of the masternode in the cluster. This is potentially causing new features to be
expected on shards that didn't exist when the index was created.
There is no notion of
where was the shard allocated first
such thatindex.version.created
can't be reliably used as a feature flag.With this change the
index.version.created
can be reliably used todetermin the smallest nodes version at the point in time when the index
was created. This means we can safely use certain features that would
for instance require reindeing and / or would not work if not the
entire index (all shards and segments) have been created with a certain
version or newer.