Skip to content

Allow shrinking of indices from a previous major #28076

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 4, 2018
Merged

Allow shrinking of indices from a previous major #28076

merged 1 commit into from
Jan 4, 2018

Conversation

@ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Jan 4, 2018

Lucene does not allow adding Lucene 6 files to a Lucene 7 index. This PR ensures that we carry over the Lucene version to the newly created Lucene index.

Closes #28061

@ywelsch ywelsch added :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. >bug critical v6.1.2 v6.2.0 v7.0.0 labels Jan 4, 2018
@ywelsch ywelsch requested a review from jpountz January 4, 2018 09:03
s1monw
s1monw suggested changes Jan 4, 2018
View reviewed changes
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.

I left one suggestion looks good otherwise

core/src/main/java/org/elasticsearch/index/shard/StoreRecovery.java
Lucene.cleanLuceneIndex(target);
assert sources.length > 0;
final int luceneIndexCreatedVersionMajor = Lucene.readSegmentInfos(sources[0]).getIndexCreatedVersionMajor();
new SegmentInfos(luceneIndexCreatedVersionMajor).commit(target);
Copy link
Contributor

@s1monw s1monw Jan 4, 2018

Choose a reason for hiding this comment

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

Can we copy the first index into the target directory instead of writing a dummy commit point? something like this:

 Directory dir = sources[0];
 for (String file : dir.listAll()) {
   target.copyFrom(dir, file, file, IOContext.DEFAULT);
  }

This way we also apply the same optimizations for hardlinking etc and we are totally safe?

Copy link
Contributor

@jpountz jpountz Jan 4, 2018

Choose a reason for hiding this comment

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

Yannick and I discussed this option first, but this needs extra care, for instance to not copy the write lock. It's also a bit more involved if we want to track statistics as well for the first source. In the end it's not clear to me which option is better.

Copy link
Contributor Author

@ywelsch ywelsch Jan 4, 2018

Choose a reason for hiding this comment

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

@s1monw I initially started with the approach that you've outlined, but found it to be more complex for the reasons that @jpountz stated. In the future, we can hopefully create an IndexWriter for an older Lucene version (@jpountz will raise this on the Lucene project).

Copy link
Contributor

@jpountz jpountz Jan 4, 2018

Choose a reason for hiding this comment

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

I was just opening the issue but I'll wait to see the conclusion here first in case we decide copying the first directory manually is still a better trade-off.

Copy link
Contributor

@s1monw s1monw Jan 4, 2018

Choose a reason for hiding this comment

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

sounds good to me!

jpountz
jpountz approved these changes Jan 4, 2018
View reviewed changes
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for tackling it.

@ywelsch ywelsch merged commit ca32519 into elastic:master Jan 4, 2018
ywelsch added a commit that referenced this pull request Jan 4, 2018
@ywelsch
Allow shrinking of indices from a previous major (#28076)
d0e804c 
Lucene does not allow adding Lucene 6 files to a Lucene 7 index. This commit ensures that we carry over the Lucene version to the newly created Lucene index.

Closes #28061
ywelsch added a commit that referenced this pull request Jan 4, 2018
@ywelsch
Allow shrinking of indices from a previous major (#28076)
f4aa568 
Lucene does not allow adding Lucene 6 files to a Lucene 7 index. This commit ensures that we carry over the Lucene version to the newly created Lucene index.

Closes #28061
martijnvg added a commit that referenced this pull request Jan 4, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
30fa47b 
* es/master: (53 commits)
  Bump compat version for local depdendent test to 6.2.0
  Pass `java.locale.providers=COMPAT` to Java 9 onwards (#28080)
  Allow shrinking of indices from a previous major (#28076)
  Remove deprecated exceptions (#28059)
  Add Writeable.Reader support to TransportResponseHandler (#28010)
  Plugins: Add plugin extension capabilities (#27881)
  Fix cluster.routing.allocation.enable and cluster.routing.rebalance.enable casing (#28037)
  [Test] Fix scores for dcg in RankEvalRequestIT and RankEvalYamlIT
  [Docs] Add note on limitation for significant_text with nested objects (#28052)
  Enable convert processor to support Long and Double. (#27957)
  Enable Wildfly tests on JDK 9 and JDK 10
  [Test] Fix allowed delta for calculated scores in DiscountedCumulativeGainTests
  [Test] Mute DiscountedCumulativeGainTests on ARM
  Only bind loopback addresses when binding to local
  Fix assertion in Wildfly build
  Fix typo in comment in Wildfly build
  Use ephemeral ports in Wildfly tests
  Update fuzzy-query.asciidoc (#28032)
  Just another elasticsearch library (#27996)
  Disable nio test transport (#28028)
  ...
martijnvg added a commit that referenced this pull request Jan 4, 2018
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
564eee0 
* es/6.x: (48 commits)
  Bump compat version for local depdendent test to 6.2.0
  Pass `java.locale.providers=COMPAT` to Java 9 onwards (#28080)
  Allow shrinking of indices from a previous major (#28076)
  Add Writeable.Reader support to TransportResponseHandler (#28010)
  Fix cluster.routing.allocation.enable and cluster.routing.rebalance.enable casing (#28037)
  [Test] Fix scores for dcg in RankEvalRequestIT and RankEvalYamlIT
  [Docs] Add note on limitation for significant_text with nested objects (#28052)
  [Test] Fix allowed delta for calculated scores in DiscountedCumulativeGainTests
  Enable convert processor to support Long and Double. (#27957)
  Enable Wildfly tests on JDK 9 and JDK 10
  update ingest-attachment to use Tika 1.17 and newer deps (#27824)
  Only bind loopback addresses when binding to local
  Fix assertion in Wildfly build
  Fix typo in comment in Wildfly build
  Use ephemeral ports in Wildfly tests
  Update fuzzy-query.asciidoc (#28032)
  Add node id to shard failure message (#28024)
  Introduce limit to the number of terms in Terms Query (#27968)
  Upgrade Gradle Shadow plugin to 2.0.2
  Upgrade to JMH 1.19
  ...
jasontedor added a commit that referenced this pull request Jan 4, 2018
@jasontedor
Merge branch 'master' into compile-with-jdk-9
4e137e5 
* master:
  Set the elasticsearch-nio codebase for tests (#28067)
  Bump compat version for local depdendent test to 6.2.0
  Pass `java.locale.providers=COMPAT` to Java 9 onwards (#28080)
  Allow shrinking of indices from a previous major (#28076)
  Remove deprecated exceptions (#28059)
  Add Writeable.Reader support to TransportResponseHandler (#28010)
  Plugins: Add plugin extension capabilities (#27881)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 6, 2018
@jasontedor
Merge branch 'master' into mrjar
2522787 
* master: (25 commits)
  Remove Gradle cheatsheet
  Fix reproduction info to point to Gradle wrapper
  Update platforms tests to use Gradle wrapper
  Update testing docs to reflect Gradle wrapper
  Painless: Modify Loader to Load Classes Directly from Definition (elastic#28088)
  Update contributing docs to use the Gradle wrapper
  Create nio-transport plugin for NioTransport (elastic#27949)
  test: replaced try-catch statements with expectThrows(...)
  Add getWarmer and getTranslog method to NodeIndicesStats (elastic#28092)
  fix doc mistake
  Added ASN support for Ingest GeoIP plugin.
  Fix global aggregation that requires breadth first and scores (elastic#27942)
  Introduce Gradle wrapper
  Ignore GIT_COMMIT when calculating commit hash
  Re-enable bwc tests after elastic#27881 was backported
  Set the elasticsearch-nio codebase for tests (elastic#28067)
  Bump compat version for local depdendent test to 6.2.0
  Pass `java.locale.providers=COMPAT` to Java 9 onwards (elastic#28080)
  Allow shrinking of indices from a previous major (elastic#28076)
  Remove deprecated exceptions (elastic#28059)
  ...
@jpountz jpountz mentioned this pull request Sep 18, 2018
Closed
@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

Reviewers

2 more reviewers

@jpountz jpountz jpountz approved these changes

@s1monw s1monw s1monw approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

>bug critical :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. v6.1.2 v6.2.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ywelsch @jpountz @s1monw @jimczi