Skip to content
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

Merged
merged 1 commit into from Jul 1, 2014

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Jul 1, 2014

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.

@@ -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());
Copy link
Contributor

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?

Copy link
Contributor Author

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...

@kimchy
Copy link
Member

kimchy commented Jul 1, 2014

LGTM except for the comment on using the currentState var

@s1monw
Copy link
Contributor Author

s1monw commented Jul 1, 2014

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));
Copy link
Contributor

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?

Copy link
Contributor Author

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 :)

Copy link
Contributor

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.

@s1monw
Copy link
Contributor Author

s1monw commented Jul 1, 2014

and another commit now really using the currentState

@kimchy
Copy link
Member

kimchy commented Jul 1, 2014

LGTM

@bleskes
Copy link
Contributor

bleskes commented Jul 1, 2014

LGTM, left one minor comment.

@s1monw
Copy link
Contributor Author

s1monw commented Jul 1, 2014

@bleskes pushed a new commit

@bleskes
Copy link
Contributor

bleskes commented Jul 1, 2014

+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
@s1monw s1monw merged commit c9b7bec into elastic:master Jul 1, 2014
s1monw added a commit that referenced this pull request Jul 1, 2014
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
@s1monw s1monw deleted the consistent_created_version branch July 1, 2014 16:12
@s1monw s1monw added resiliency and removed review labels Jul 5, 2014
@clintongormley clintongormley changed the title [INDEX] Ensure index.version.created is consistent Resiliency: Ensure index.version.created is consistent Jul 16, 2014
@clintongormley clintongormley changed the title Resiliency: Ensure index.version.created is consistent Ensure index.version.created is consistent Jun 7, 2015
@clintongormley clintongormley added :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. and removed :Cluster labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. >enhancement resiliency v1.3.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants