Deprecate not copy settings and explicitly disallow #30404

jasontedor · 2018-05-04T22:55:50Z

We want copying settings to be the default behavior. This commit deprecates not copying settings, and disallows explicitly not copying settings. This gives users a transition path to the future default behavior.

Relates #28347, relates #30255, supersedes #30321

We want copying settings to be the default behavior. This commit deprecates not copying settings, and disallows explicitly not copying settings. This gives users a transition path to the future default behavior.

elasticmachine · 2018-05-04T22:55:51Z

Pinging @elastic/es-core-infra

…r-you * origin/master: Update forcemerge.asciidoc (elastic#30113) Added zentity to the list of API extension plugins (elastic#29143) Fix the search request default operation behavior doc (elastic#29302) (elastic#29405) Watcher: Mark watcher as started only after loading watches (elastic#30403) Pass the task to broadcast actions (elastic#29672) Disable REST default settings testing until elastic#29229 is back-ported Correct wording in log message (elastic#30336) Do not fail snapshot when deleting a missing snapshotted file (elastic#30332) AwaitsFix testCreateShrinkIndexToN DOCS: Correct mapping tags in put-template api DOCS: Fix broken link in the put index template api Add put index template api to high level rest client (elastic#30400) Relax testAckedIndexing to allow document updating [Docs] Add snippets for POS stop tags default value Move respect accept header on no handler to 6.3.1

talevy

looks good! just had one nit about deprecation description

talevy · 2018-05-07T17:56:24Z

docs/reference/indices/shrink-index.asciidoc

@@ -125,9 +134,11 @@ NOTE: By default, with the exception of `index.analysis`, `index.similarity`,
 and `index.sort` settings, index settings on the source index are not copied
 during a shrink operation. With the exception of non-copyable settings, settings
 from the source index can be copied to the target index by adding the URL
-parameter `copy_settings=true` to the request.
+parameter `copy_settings=true` to the request. Note that `copy_settings` can not


should this be cannot? I feel like the meaning of this as written states
that it can be set to something other than false, while writing cannot makes it clear that
false is not allowed.

I'm confused @talevy? I thought that "can not" and "cannot" can be used interchangeably?

I guess it is up to the reader. I just find it ambiguous since it takes on a different meaning when it proceeds expressions that have "not" in them.

It can not only be used this way, but another way as well.

If you see both interchangeable in that example, then I am probably wrong

I agree with @jasontedor that cannot and can not can be used interchangably

talevy · 2018-05-07T17:57:13Z

docs/reference/indices/split-index.asciidoc

@@ -181,9 +181,11 @@ NOTE: By default, with the exception of `index.analysis`, `index.similarity`,
 and `index.sort` settings, index settings on the source index are not copied
 during a split operation. With the exception of non-copyable settings, settings
 from the source index can be copied to the target index by adding the URL
-parameter `copy_settings=true` to the request.
+parameter `copy_settings=true` to the request. Note that `copy_settings` can not


same as above ^

* master: Fix line length violation in cache tests Add stricter geohash parsing (elastic#30376) Add failing test for core cache deadlock [DOCS] convert forcemerge snippet

colings86

@jasontedor I left some questions, I don't think any of them are major things though

colings86 · 2018-05-08T09:50:37Z

docs/reference/indices/shrink-index.asciidoc

@@ -125,9 +134,11 @@ NOTE: By default, with the exception of `index.analysis`, `index.similarity`,
 and `index.sort` settings, index settings on the source index are not copied
 during a shrink operation. With the exception of non-copyable settings, settings
 from the source index can be copied to the target index by adding the URL
-parameter `copy_settings=true` to the request.
+parameter `copy_settings=true` to the request. Note that `copy_settings` can not


I agree with @jasontedor that cannot and can not can be used interchangably

colings86 · 2018-05-08T09:59:08Z

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java

@@ -80,6 +80,9 @@ public ActionRequestValidationException validate() {
        if (type == ResizeType.SPLIT && IndexMetaData.INDEX_NUMBER_OF_SHARDS_SETTING.exists(targetIndexRequest.settings()) == false) {
            validationException = addValidationError("index.number_of_shards is required for split operations", validationException);
        }
+        if (copySettings != null && copySettings == false) {
+            validationException = addValidationError("[copySettings] can not be explicitly set to [false]", validationException);
+        }


Should we instead do this validation on the setter method? That way:

The stack trace that is thrown will contain the call stack that actually tried to set copySettings in an invalid way making it easier to debug

Allows us to put the validation only on the setter method (and have the rest handler call the setter method) so we ensure both the rest and transport validation are consistent and fail early.

colings86 · 2018-05-08T10:00:35Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestResizeHandler.java

        if (rawCopySettings == null) {
            copySettings = resizeRequest.getCopySettings();
+        } else if (rawCopySettings.isEmpty()) {
+            copySettings = true;


Is this right? It seems like this would make the copySettings default true for the rest layer but false for the transport layer?

Could we not let TransportResizeAction be the only thing that handles the null case? So to change the default from false to true in 8.x we only need to change it in that one place?

That's not what this means, this is the non-null but empty case for copy_settings. This handles the case when a user invokes /source/_shrink/target?copy_settings; we have a convention in our APIs that ?parameter without =true or =false means true (e.g., `?pretty).

I am also okay with some duplication for temporary code that is scoped. This code in master is going to immediately be modified so that we default to true.

Also, I prefer to fail early on the REST layer rather than waiting for a round-trip to the master and resulting in a remote transport exception.

Also, I prefer to fail early on the REST layer rather than waiting for a round-trip to the master and resulting in a remote transport exception.

That was my point with https://github.com/elastic/elasticsearch/pull/30404/files/6c0a153bc037df67dcca2b036dcb367e3f084081#r186675743 of doing the validation in the setCopySettings method and throwing an IllegalArgumentException there if the value passed in is false. It means we only have the validation in one place and the exception is thrown at the point the value is set (whether from the REST layer or the transport client).

Right, but then we can not provide a clear error message. On the REST layer we want to tell the user that copy_settings can not be false, on the transport layer we want to tell the user that copySettings can not be false. I am fine moving the transport validation to the setter but I think the REST validation should remain?

@colings86 I pushed.

…or-you * elastic/master: (22 commits) Docs: Test examples that recreate lang analyzers (elastic#29535) BulkProcessor to retry based on status code (elastic#29329) Add GET Repository High Level REST API (elastic#30362) add a comment explaining the need for RetryOnReplicaException on missing mappings Add `coordinating_only` node selector (elastic#30313) Stop forking groovyc (elastic#30471) Avoid setting connection request timeout (elastic#30384) Use date format in `date_range` mapping before fallback to default (elastic#29310) Watcher: Increase HttpClient parallel sent requests (elastic#30130) Mute ML upgrade test (elastic#30458) Stop forking javac (elastic#30462) Client: Deprecate many argument performRequest (elastic#30315) Docs: Use task_id in examples of tasks (elastic#30436) Security: Rename IndexLifecycleManager to SecurityIndexManager (elastic#30442) [Docs] Fix typo in cardinality-aggregation.asciidoc (elastic#30434) Avoid NPE in `more_like_this` when field has zero tokens (elastic#30365) Build: Switch to building javadoc with html5 (elastic#30440) Add a quick tour of the project to CONTRIBUTING (elastic#30187) Reindex: Use request flavored methods (elastic#30317) Silence SplitIndexIT.testSplitIndexPrimaryTerm test failure. (elastic#30432) ...

colings86

LGTM

* master: (41 commits) Bump Gradle heap to 2 GB (elastic#30535) SQL: Use request flavored methods in tests (elastic#30345) Suppress hdfsFixture if there are spaces in the path (elastic#30302) Delete temporary blobs before creating index file (elastic#30528) Watcher: Remove TriggerEngine.getJobCount() (elastic#30395) [ML] Fix wire BWC for JobUpdate (elastic#30512) Use simpler write-once semantics for FS repository (elastic#30435) Derive max composite buffers from max content len Use simpler write-once semantics for HDFS repository (elastic#30439) SQL: Improve correctness of SYS COLUMNS & TYPES (elastic#30418) Mute two tests in FlushIT with @AwaitsFix. Fix incorrect template name in test case Build: Remove legacy bwc files from xpack (elastic#30485) Mute UnicastZenPingTests#testSimplePings with @AwaitsFix. Security: cleanup code in file stores (elastic#30348) Security: fix TokenMetaData equals and hashcode (elastic#30347) Mute two tests from SmokeTestWatcherWithSecurityClientYamlTestSuiteIT. Mute SharedClusterSnapshotRestoreIT#testSnapshotSucceedsAfterSnapshotFailure with @AwaitsFix. SQL: Improve compatibility with MS query (elastic#30516) SQL: Fix parsing of dates with milliseconds (elastic#30419) ...

* master: Deprecate not copy settings and explicitly disallow (elastic#30404) [ML] Improve state persistence log message Build: Add mavenPlugin cluster configuration method (elastic#30541) Re-enable FlushIT tests Bump Gradle heap to 2 GB (elastic#30535) SQL: Use request flavored methods in tests (elastic#30345)

We want copying settings to be the default behavior. This commit deprecates not copying settings, and disallows explicitly not copying settings. This gives users a transition path to the future default behavior.

* master: Default to one shard (#30539) Unmute IndexUpgradeIT tests Forbid expensive query parts in ranking evaluation (#30151) Docs: Update HighLevelRestClient migration docs (#30544) Clients: Switch to new performRequest (#30543) [TEST] Fix typo in MovAvgIT test Add missing dependencies on testClasses (#30527) [TEST] Mute ML test that needs updating to following ml-cpp changes Document woes between auto-expand-replicas and allocation filtering (#30531) Moved tokenizers to analysis common module (#30538) Adjust copy settings versions Mute ShrinkIndexIT suite SQL: SYS TABLES ordered according to *DBC specs (#30530) Deprecate not copy settings and explicitly disallow (#30404) [ML] Improve state persistence log message Build: Add mavenPlugin cluster configuration method (#30541) Re-enable FlushIT tests Bump Gradle heap to 2 GB (#30535) SQL: Use request flavored methods in tests (#30345) Suppress hdfsFixture if there are spaces in the path (#30302) Delete temporary blobs before creating index file (#30528) Watcher: Remove TriggerEngine.getJobCount() (#30395) [ML] Fix wire BWC for JobUpdate (#30512) Use simpler write-once semantics for FS repository (#30435) Derive max composite buffers from max content len Use simpler write-once semantics for HDFS repository (#30439) SQL: Improve correctness of SYS COLUMNS & TYPES (#30418) Mute two tests in FlushIT with @AwaitsFix. Fix incorrect template name in test case Build: Remove legacy bwc files from xpack (#30485) Mute UnicastZenPingTests#testSimplePings with @AwaitsFix. Security: cleanup code in file stores (#30348) Security: fix TokenMetaData equals and hashcode (#30347) Mute two tests from SmokeTestWatcherWithSecurityClientYamlTestSuiteIT. Mute SharedClusterSnapshotRestoreIT#testSnapshotSucceedsAfterSnapshotFailure with @AwaitsFix. SQL: Improve compatibility with MS query (#30516) SQL: Fix parsing of dates with milliseconds (#30419)

* 6.x: Unmute IndexUpgradeIT tests Forbid expensive query parts in ranking evaluation (#30151) Docs: Update HighLevelRestClient migration docs (#30544) Clients: Switch to new performRequest (#30543) [TEST] Fix typo in MovAvgIT test [TEST] Mute ML test that needs updating to following ml-cpp changes Moved tokenizers to analysis common module (#30538) Document woes between auto-expand-replicas and allocation filtering (#30531) [ML] Hide internal Job update options from the REST API (#30537) Deprecate not copy settings and explicitly disallow (#30404) Mute ShrinkIndexIT suite SQL: SYS TABLES ordered according to *DBC specs (#30530) [ML] Improve state persistence log message Build: Add mavenPlugin cluster configuration method (#30541) Re-enable FlushIT tests Bump Gradle heap to 2 GB (#30535) Bump Gradle heap to 1792m (#30484) SQL: Use request flavored methods in tests (#30345) Suppress hdfsFixture if there are spaces in the path (#30302) Delete temporary blobs before creating index file (#30528) Watcher: Remove TriggerEngine.getJobCount() (#30395) Use simpler write-once semantics for FS repository (#30435) Use simpler write-once semantics for HDFS repository (#30439) SQL: Improve correctness of SYS COLUMNS & TYPES (#30418) Mute two tests in FlushIT with @AwaitsFix. Fix incorrect template name in test case Build: Remove legacy bwc files from xpack (#30485) Security: Simplify security index listeners (#30466) Mute SharedClusterSnapshotRestoreIT#testSnapshotSucceedsAfterSnapshotFailure with @AwaitsFix. Add proper longitude validation in geo_polygon_query (#30497) Mute UnicastZenPingTests#testSimplePings with @AwaitsFix. Security: cleanup code in file stores (#30348) Security: fix TokenMetaData equals and hashcode (#30347) Mute two tests from SmokeTestWatcherWithSecurityClientYamlTestSuiteIT. Fix incorrect merged entry in changelog SQL: Improve compatibility with MS query (#30516) SQL: Fix parsing of dates with milliseconds (#30419)

* es/ccr: (37 commits) Default to one shard (#30539) Unmute IndexUpgradeIT tests Forbid expensive query parts in ranking evaluation (#30151) Docs: Update HighLevelRestClient migration docs (#30544) Clients: Switch to new performRequest (#30543) [TEST] Fix typo in MovAvgIT test Add missing dependencies on testClasses (#30527) [TEST] Mute ML test that needs updating to following ml-cpp changes Document woes between auto-expand-replicas and allocation filtering (#30531) Moved tokenizers to analysis common module (#30538) Adjust copy settings versions Mute ShrinkIndexIT suite SQL: SYS TABLES ordered according to *DBC specs (#30530) Deprecate not copy settings and explicitly disallow (#30404) [ML] Improve state persistence log message Build: Add mavenPlugin cluster configuration method (#30541) Re-enable FlushIT tests Bump Gradle heap to 2 GB (#30535) SQL: Use request flavored methods in tests (#30345) Suppress hdfsFixture if there are spaces in the path (#30302) ...

* es/ccr: (37 commits) Default to one shard (elastic#30539) Unmute IndexUpgradeIT tests Forbid expensive query parts in ranking evaluation (elastic#30151) Docs: Update HighLevelRestClient migration docs (elastic#30544) Clients: Switch to new performRequest (elastic#30543) [TEST] Fix typo in MovAvgIT test Add missing dependencies on testClasses (elastic#30527) [TEST] Mute ML test that needs updating to following ml-cpp changes Document woes between auto-expand-replicas and allocation filtering (elastic#30531) Moved tokenizers to analysis common module (elastic#30538) Adjust copy settings versions Mute ShrinkIndexIT suite SQL: SYS TABLES ordered according to *DBC specs (elastic#30530) Deprecate not copy settings and explicitly disallow (elastic#30404) [ML] Improve state persistence log message Build: Add mavenPlugin cluster configuration method (elastic#30541) Re-enable FlushIT tests Bump Gradle heap to 2 GB (elastic#30535) SQL: Use request flavored methods in tests (elastic#30345) Suppress hdfsFixture if there are spaces in the path (elastic#30302) ...

Deprecate not copy settings and explicitly disallow

e87c9d8

We want copying settings to be the default behavior. This commit deprecates not copying settings, and disallows explicitly not copying settings. This gives users a transition path to the future default behavior.

jasontedor added review :Data Management/Indices APIs APIs to create and manage indices and templates v7.0.0 v6.4.0 labels May 4, 2018

jasontedor requested review from colings86 and talevy May 4, 2018 22:55

Adjust docs

add8989

jasontedor mentioned this pull request May 6, 2018

Remove setting copy settings on resize to false #30321

Closed

jasontedor added 2 commits May 6, 2018 18:57

Fix docs test

56ceb89

talevy approved these changes May 7, 2018

View reviewed changes

jasontedor added 2 commits May 7, 2018 14:46

Merge branch 'master' into no-copy-settings-for-you

d756e66

* master: Fix line length violation in cache tests Add stricter geohash parsing (elastic#30376) Add failing test for core cache deadlock [DOCS] convert forcemerge snippet

Newlines

6c0a153

colings86 requested changes May 8, 2018

View reviewed changes

jasontedor added 2 commits May 9, 2018 09:43

Move validation

2c20228

colings86 approved these changes May 11, 2018

View reviewed changes

jasontedor added 2 commits May 11, 2018 20:21

Fix merge from master

6aabd70

jasontedor merged commit 593fdd4 into elastic:master May 13, 2018

jasontedor mentioned this pull request May 14, 2018

Adjust versions for resize copy settings #30578

Merged

jasontedor added the >deprecation label Aug 20, 2018

jasontedor deleted the no-copy-settings-for-you branch September 1, 2018 12:37

Mpdreamz mentioned this pull request Sep 20, 2018

[meta] 6.4.0 release elastic/elasticsearch-net#3397

Closed

89 tasks

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate not copy settings and explicitly disallow #30404

Deprecate not copy settings and explicitly disallow #30404

jasontedor commented May 4, 2018

elasticmachine commented May 4, 2018

talevy left a comment

talevy May 7, 2018

jasontedor May 7, 2018

talevy May 7, 2018

colings86 May 8, 2018

talevy May 7, 2018

colings86 left a comment

colings86 May 8, 2018

colings86 May 8, 2018

colings86 May 8, 2018

jasontedor May 9, 2018

jasontedor May 9, 2018

colings86 May 9, 2018

jasontedor May 9, 2018

colings86 May 9, 2018

jasontedor May 9, 2018

colings86 left a comment

Deprecate not copy settings and explicitly disallow #30404

Deprecate not copy settings and explicitly disallow #30404

Conversation

jasontedor commented May 4, 2018

elasticmachine commented May 4, 2018

talevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colings86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colings86 left a comment

Choose a reason for hiding this comment