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

Introduce index settings version #34429

Merged
merged 17 commits into from
Oct 16, 2018
Merged

Conversation

jasontedor
Copy link
Member

This commit introduces settings version to index metadata. This value is monotonically increasing and is updated on settings updates. This will be useful in cross-cluster replication so that we can request settings updates from the leader only when there is a settings update.

This commit introduces settings version to index metadata. This value is
monotonically increasing and is updated on settings updates. This will
be useful in cross-cluster replication so that we can request settings
updates from the leader only when there is a settings update.
@jasontedor jasontedor added the :Data Management/Indices APIs APIs to create and manage indices and templates label Oct 13, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jasontedor jasontedor added v7.0.0 v6.5.0 :Distributed/CCR Issues around the Cross Cluster State Replication features labels Oct 13, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

this LGTM - I wonder if we should make this entire version game here in IMD less error prone. I don't have a good answer but now we have 3 version that are all can be changed and incremented separately I do wonder if we should streamline it at least with a Version class that only can move forward?

public Builder mappingVersion(final long mappingVersion) {
this.mappingVersion = mappingVersion;
return this;
}

public Builder settingsVersion(final long settingsVersion) {
this.settingsVersion = settingsVersion;
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to check that we always increase?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would not be correct since this method is called when building index metadata from parsing or de-serializing.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

This looks good. I also do think that we need to update the index settings version in IndexMetaData.Builder#numberOfReplicas(...) method. This is used by AllocationService#adaptAutoExpandReplicas(...) and I think the reason the UpdateNumberOfReplicasIT fails.

if (same(currentState.metaData().index(index).getSettings(), metaDataBuilder.get(index).getSettings()) == false) {
final IndexMetaData.Builder builder = IndexMetaData.builder(metaDataBuilder.get(index));
builder.settingsVersion(1 + builder.settingsVersion());
metaDataBuilder.put(builder);
Copy link
Member

Choose a reason for hiding this comment

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

can we instead increment the version at lines 169 and 185? like:

metaDataBuilder.put(IndexMetaData.builder(indexMetaData).settings(finalSettings).settingsVersion(1 + builder.settingsVersion()));

At these places we do know that the index versions have been updated. And then we don't have to check whether settings have been changed in another for loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would not be correct since a settings update does not necessarily mutate any settings. The test UpdateSettingsIT#testSettingsVersionUnchanged test for this case. That is, a settings update that does not change any settings should not modify the settings version.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for adding that test!

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM

* elastic/master:
  Mute PartitionedRoutingIT#testShrinking on Windows
  Mute testToQuery test
  [TEST] Make sure there are shards started so that `ESIntegTestCase#assertSameDocIdsOnShards()` does not fail with shard not found.
  Change shard changes api's threadpool from get to search (elastic#34421)
  Update TESTING.asciidoc title (elastic#34401)
  Tests: Fix DateFormatter equals tests with locale (elastic#34435)
  Docs: Remove unnecessary qualifier from wildcard import note (elastic#34419)
  CCR/TEST: AwaitsFix testFailOverOnFollower
  [Painless] Add a Map for java names to classes for use in the custom classloader (elastic#34424)
  TEST: Fix indentation in FullClusterRestartIT (elastic#34420)
  [WIP] Ingest Attachement: Upgrade tika to v1.19.1 (elastic#33896)
  NETWORKING: Upgrade Netty to 4.1.30 (elastic#34417)
  Allow an AuthenticationResult to return metadata (elastic#34382)
  [ML] Add an ingest pipeline definition to structure finder (elastic#34350)
  Handle pre-6.x time fields (elastic#34373)
  ListenableFuture should preserve ThreadContext (elastic#34394)
@jasontedor
Copy link
Member Author

This test failure here is fixed by #34481.

* master:
  Do not update number of replicas on no indices (elastic#34481)
  Core: Remove two methods from AbstractComponent (elastic#34336)
  Use RoleRetrievalResult for better caching (elastic#34197)
  Revert "Search: Fix spelling mistake in Javadoc (elastic#34480)"
  Search: Fix spelling mistake in Javadoc (elastic#34480)
  Docs: improve formatting of Query String Query doc page (elastic#34432)
  Tests: Handle epoch date formatters edge cases (elastic#34437)
  Test: Fix running with external cluster
  Fix handling of empty keyword in terms aggregation (elastic#34457)
  [DOCS] Fix tag in SecurityDocumentationIT
  [Monitoring] Add additional necessary mappings for apm-server (elastic#34392)
  SCRIPTING: Move Aggregation Script Context to its own class (elastic#33820)
  MINOR: Remove Deadcode in  ExpressionTermSetQuery (elastic#34442)
  HLRC: Get SSL Certificates API (elastic#34135)
@jasontedor jasontedor merged commit 4b2052c into elastic:master Oct 16, 2018
jasontedor added a commit that referenced this pull request Oct 16, 2018
This commit introduces settings version to index metadata. This value is
monotonically increasing and is updated on settings updates. This will
be useful in cross-cluster replication so that we can request settings
updates from the leader only when there is a settings update.
@jasontedor jasontedor deleted the settings-version branch October 16, 2018 10:39
kcm pushed a commit that referenced this pull request Oct 30, 2018
This commit introduces settings version to index metadata. This value is
monotonically increasing and is updated on settings updates. This will
be useful in cross-cluster replication so that we can request settings
updates from the leader only when there is a settings update.
@colings86 colings86 removed the :Distributed/CCR Issues around the Cross Cluster State Replication features label Nov 2, 2018
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Indices APIs APIs to create and manage indices and templates >enhancement v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants