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

Reject setting index.optimize_auto_generated_id after version 7.0.0 #28895

Merged
merged 8 commits into from
Feb 10, 2019

Conversation

liketic
Copy link

@liketic liketic commented Mar 4, 2018

Do not allow modify setting index.optimize_auto_generated_id after version 7.0.

Relates to #27600

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Copy link
Member

@jasontedor jasontedor 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 a comment about the location of the test.

import org.elasticsearch.test.IndexSettingsModule;


public class EngineConfigTests extends ESTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

I do not think we need a new class for this. Maybe there is an existing class where this would be appropriate?

Copy link
Author

@liketic liketic Mar 5, 2018

Choose a reason for hiding this comment

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

Thanks @jasontedor , how about elasticsearch/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java or elasticsearch/server/src/test/java/org/elasticsearch/index/engine/InternalEngineSettingsTests.java ?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds better.

Copy link
Author

Choose a reason for hiding this comment

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

I moved the test to InternalEngineTests.java. I'm not sure if it's appropriate. Please review again, thanks!

@colings86 colings86 added :Data Management/Indices APIs APIs to create and manage indices and templates >deprecation labels Apr 24, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@javanna
Copy link
Member

javanna commented May 7, 2018

Review comment seems to have been addressed, @jasontedor would you mind having another look?

@javanna javanna added the review label May 7, 2018
@jasontedor
Copy link
Member

@liketic The change looks good; can you merge master and resolve conflicts and we can integrate this?

@liketic
Copy link
Author

liketic commented May 26, 2018

Thanks @jasontedor , please review again.

@jasontedor
Copy link
Member

@elasticmachine test this please

@javanna
Copy link
Member

javanna commented Aug 16, 2018

@jasontedor does it make sense to try and resurrect this one together with #28862?

@rjernst rjernst removed the review label Oct 10, 2018
@jasontedor
Copy link
Member

@liketic Can you bring this PR up to date with master? I tried to merge master in and didn't merge cleanly.

@liketic
Copy link
Author

liketic commented Dec 13, 2018

Thanks @jasontedor , please review again.

Copy link
Member

@jasontedor jasontedor 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 a comment. Let me know if you need a pointer to the right place to reject this setting.

@@ -126,6 +127,11 @@ public EngineConfig(ShardId shardId, String allocationId, ThreadPool threadPool,
List<ReferenceManager.RefreshListener> internalRefreshListener, Sort indexSort,
CircuitBreakerService circuitBreakerService, LongSupplier globalCheckpointSupplier,
LongSupplier primaryTermSupplier, TombstoneDocSupplier tombstoneDocSupplier) {
if (indexSettings.getIndexVersionCreated().onOrAfter(Version.V_7_0_0)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is too late to be rejecting this setting. It means the index will already be created and in the cluster state, but all shards will fail during engine creation. The user will be left with a red index.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. I moved to createIndexService. Could you please review again?

* master: (1159 commits)
  Fix timezone fallback in ingest processor (elastic#38407)
  Avoid polluting download stats on builds (elastic#38660)
  SQL: Prevent grouping over grouping functions (elastic#38649)
  SQL: Relax StackOverflow circuit breaker for constants (elastic#38572)
  [DOCS] Fixes broken migration links (elastic#38655)
  Drop support for the low-level REST client on JDK 7 (elastic#38540)
  [DOCS] Adds placeholders for v8 highlights, breaking changes, release notes (elastic#38641)
  fix dissect doc "ip" --> "clientip" (elastic#38545)
  Concurrent file chunk fetching for CCR restore (elastic#38495)
  make DateMathIndexExpressionsIntegrationIT more resilient (elastic#38473)
  SQL: Replace joda with java time (elastic#38437)
  Add fuzziness example (elastic#37194) (elastic#38648)
  Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636)
  add geotile_grid ref to asciidoc (elastic#38632)
  Enable Dockerfile from artifacts.elastic.co (elastic#38552)
  Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634)
  Account for a possible rolled over file while reading the audit log file (elastic#34909)
  Mute failure in InternalEngineTests (elastic#38622)
  Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518)
  Refactor ZonedDateTime.now in millis resolution (elastic#38577)
  ...
@jasontedor
Copy link
Member

@elasticmachine test this please

@jasontedor jasontedor merged commit fe5bdb4 into elastic:master Feb 10, 2019
jasontedor pushed a commit that referenced this pull request Feb 10, 2019
This commit rejects the index.optmize_auto_generated_id setting for
indices created on or after 7.0.0. This setting was deprecated in 6.7.0.
jasontedor pushed a commit that referenced this pull request Feb 10, 2019
This commit rejects the index.optmize_auto_generated_id setting for
indices created on or after 7.0.0. This setting was deprecated in 6.7.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants