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

Deprecate not copy settings and explicitly disallow #30404

Merged
merged 10 commits into from
May 13, 2018
3 changes: 2 additions & 1 deletion docs/CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ analysis module. ({pull}30397[#30397])
[float]
=== Enhancements

{ref-64}/breaking_64_api_changes.html#copy-source-settings-on-resize[Allow copying source settings on index resize operations] ({pull}30255[#30255])
{ref-64}/breaking_64_api_changes.html#copy-source-settings-on-resize[Allow
copying source settings on index resize operations] ({pull}30255[#30255], {pull}30404[#30404])

Added new "Request" object flavored request methods. Prefer these instead of the
multi-argument versions. ({pull}29623[#29623])
Expand Down
19 changes: 15 additions & 4 deletions docs/reference/indices/shrink-index.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,20 @@ the following request:

[source,js]
--------------------------------------------------
POST my_source_index/_shrink/my_target_index
POST my_source_index/_shrink/my_target_index?copy_settings=true
{
"settings": {
"index.routing.allocation.require._name": null, <1>
"index.blocks.write": null <2>
}
}
--------------------------------------------------
// CONSOLE
// TEST[continued]

<1> Clear the allocation requirement copied from the source index.
<2> Clear the index write block copied from the source index.

The above request returns immediately once the target index has been added to
the cluster state -- it doesn't wait for the shrink operation to start.

Expand Down Expand Up @@ -97,7 +106,7 @@ and accepts `settings` and `aliases` parameters for the target index:

[source,js]
--------------------------------------------------
POST my_source_index/_shrink/my_target_index
POST my_source_index/_shrink/my_target_index?copy_settings=true
{
"settings": {
"index.number_of_replicas": 1,
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

be set to `false`. The parameter `copy_settings` will be removed in 9.0.0

deprecated[6.4.0, `copy_settings` will default to `true` in 8.x and will be removed in 9.0.0]
deprecated[6.4.0, not copying settings is deprecated, copying settings will be
the default behavior in 8.x]

[float]
=== Monitoring the shrink process
Expand Down
10 changes: 6 additions & 4 deletions docs/reference/indices/split-index.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ the following request:

[source,js]
--------------------------------------------------
POST my_source_index/_split/my_target_index
POST my_source_index/_split/my_target_index?copy_settings=true
{
"settings": {
"index.number_of_shards": 2
Expand Down Expand Up @@ -158,7 +158,7 @@ and accepts `settings` and `aliases` parameters for the target index:

[source,js]
--------------------------------------------------
POST my_source_index/_split/my_target_index
POST my_source_index/_split/my_target_index?copy_settings=true
{
"settings": {
"index.number_of_shards": 5 <1>
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above ^

be set to `false`. The parameter `copy_settings` will be removed in 9.0.0

deprecated[6.4.0, `copy_settings` will default to `true` in 8.x and will be removed in 9.0.0]
deprecated[6.4.0, not copying settings is deprecated, copying settings will be
the default behavior in 8.x]

[float]
=== Monitoring the split process
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
---
"Shrink index via API":
- skip:
version: " - 6.99.99"
reason: expects warnings that pre-7.0.0 will not send
features: "warnings"
# creates an index with one document solely allocated on the master node
# and shrinks it into a new index with a single shard
# we don't do the relocation to a single node after the index is created
Expand Down Expand Up @@ -62,6 +66,8 @@
body:
settings:
index.number_of_replicas: 0
warnings:
- "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"

- do:
cluster.health:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
---
"Shrink index ignores target template mapping":
- skip:
version: " - 6.99.99"
reason: expects warnings that pre-7.0.0 will not send
features: "warnings"

- do:
cluster.state: {}
# Get master node id
Expand Down Expand Up @@ -65,6 +70,8 @@
body:
settings:
index.number_of_replicas: 0
warnings:
- "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"

- do:
cluster.health:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
---
"Copy settings during shrink index":
- skip:
version: " - 6.3.99"
reason: copy_settings did not exist prior to 6.4.0
version: " - 6.99.99"
reason: expects warnings that pre-7.0.0 will not send
features: "warnings"

- do:
Expand Down Expand Up @@ -47,8 +47,6 @@
settings:
index.number_of_replicas: 0
index.merge.scheduler.max_thread_count: 2
warnings:
- "parameter [copy_settings] is deprecated but was [true]"

- do:
cluster.health:
Expand All @@ -64,20 +62,19 @@
- match: { copy-settings-target.settings.index.blocks.write: "true" }
- match: { copy-settings-target.settings.index.routing.allocation.include._id: $master }

# now we do a actual shrink and do not copy settings
# now we do a actual shrink and do not copy settings (by default)
- do:
indices.shrink:
index: "source"
target: "no-copy-settings-target"
wait_for_active_shards: 1
master_timeout: 10s
copy_settings: false
body:
settings:
index.number_of_replicas: 0
index.merge.scheduler.max_thread_count: 2
warnings:
- "parameter [copy_settings] is deprecated but was [false]"
- "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"

- do:
cluster.health:
Expand All @@ -92,3 +89,16 @@
- match: { no-copy-settings-target.settings.index.merge.scheduler.max_thread_count: "2" }
- is_false: no-copy-settings-target.settings.index.blocks.write
- is_false: no-copy-settings-target.settings.index.routing.allocation.include._id

# now we do a actual shrink and try to set no copy settings
- do:
catch: /illegal_argument_exception/
indices.shrink:
index: "source"
target: "explicit-no-copy-settings-target"
wait_for_active_shards: 1
master_timeout: 10s
copy_settings: false
body:
settings:
index.number_of_replicas: 0
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ setup:
---
"Split index via API":
- skip:
version: " - 6.0.99"
reason: Added in 6.1.0
version: " - 6.99.99"
reason: expects warnings that pre-7.0.0 will not send
features: "warnings"

# make it read-only
- do:
Expand All @@ -60,6 +61,8 @@ setup:
settings:
index.number_of_replicas: 0
index.number_of_shards: 4
warnings:
- "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"

- do:
cluster.health:
Expand Down Expand Up @@ -105,7 +108,8 @@ setup:
"Split from 1 to N":
- skip:
version: " - 6.99.99"
reason: Added in 7.0.0
reason: expects warnings that pre-7.0.0 will not send
features: "warnings"
- do:
indices.create:
index: source_one_shard
Expand Down Expand Up @@ -159,6 +163,8 @@ setup:
settings:
index.number_of_replicas: 0
index.number_of_shards: 5
warnings:
- "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"

- do:
cluster.health:
Expand Down Expand Up @@ -204,8 +210,9 @@ setup:
---
"Create illegal split indices":
- skip:
version: " - 6.0.99"
reason: Added in 6.1.0
version: " - 6.99.99"
reason: expects warnings that pre-7.0.0 will not send
features: "warnings"

# try to do an illegal split with number_of_routing_shards set
- do:
Expand All @@ -220,6 +227,8 @@ setup:
index.number_of_replicas: 0
index.number_of_shards: 4
index.number_of_routing_shards: 8
warnings:
- "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"

# try to do an illegal split with illegal number_of_shards
- do:
Expand All @@ -233,3 +242,5 @@ setup:
settings:
index.number_of_replicas: 0
index.number_of_shards: 6
warnings:
- "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
---
"Split index ignores target template mapping":
- skip:
version: " - 6.0.99"
reason: Added in 6.1.0
version: " - 6.99.99"
reason: expects warnings that pre-7.0.0 will not send
features: "warnings"

# create index
- do:
Expand Down Expand Up @@ -64,6 +65,8 @@
settings:
index.number_of_shards: 2
index.number_of_replicas: 0
warnings:
- "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"

- do:
cluster.health:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
---
"Copy settings during split index":
- skip:
version: " - 6.3.99"
reason: copy_settings did not exist prior to 6.4.0
version: " - 6.99.99"
reason: expects warnings that pre-7.0.0 will not send
features: "warnings"

- do:
Expand Down Expand Up @@ -50,8 +50,6 @@
index.number_of_replicas: 0
index.number_of_shards: 2
index.merge.scheduler.max_thread_count: 2
warnings:
- "parameter [copy_settings] is deprecated but was [true]"

- do:
cluster.health:
Expand All @@ -67,21 +65,20 @@
- match: { copy-settings-target.settings.index.blocks.write: "true" }
- match: { copy-settings-target.settings.index.routing.allocation.include._id: $master }

# now we do a actual shrink and do not copy settings
# now we do a actual shrink and do not copy settings (by default)
- do:
indices.split:
index: "source"
target: "no-copy-settings-target"
wait_for_active_shards: 1
master_timeout: 10s
copy_settings: false
body:
settings:
index.number_of_replicas: 0
index.number_of_shards: 2
index.merge.scheduler.max_thread_count: 2
warnings:
- "parameter [copy_settings] is deprecated but was [false]"
- "resize operations without copying settings is deprecated; set parameter [copy_settings] to [true] for future default behavior"

- do:
cluster.health:
Expand All @@ -96,3 +93,15 @@
- match: { no-copy-settings-target.settings.index.merge.scheduler.max_thread_count: "2" }
- is_false: no-copy-settings-target.settings.index.blocks.write
- is_false: no-copy-settings-target.settings.index.routing.allocation.include._id

- do:
catch: /illegal_argument_exception/
indices.split:
index: "source"
target: "explicit-no-copy-settings-target"
wait_for_active_shards: 1
master_timeout: 10s
copy_settings: false
body:
settings:
index.number_of_replicas: 0
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public class ResizeRequest extends AcknowledgedRequest<ResizeRequest> implements
private CreateIndexRequest targetIndexRequest;
private String sourceIndex;
private ResizeType type = ResizeType.SHRINK;
private boolean copySettings = false;
private Boolean copySettings;

ResizeRequest() {}

Expand All @@ -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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

return validationException;
}

Expand All @@ -98,10 +101,12 @@ public void readFrom(StreamInput in) throws IOException {
} else {
type = ResizeType.SHRINK; // BWC this used to be shrink only
}
if (in.getVersion().onOrAfter(Version.V_6_4_0)) {
if (in.getVersion().before(Version.V_6_4_0)) {
copySettings = null;
} else if (in.getVersion().onOrAfter(Version.V_6_4_0) && in.getVersion().before(Version.V_7_0_0_alpha1)){
copySettings = in.readBoolean();
} else {
copySettings = false;
copySettings = in.readOptionalBoolean();
}
}

Expand All @@ -113,8 +118,12 @@ public void writeTo(StreamOutput out) throws IOException {
if (out.getVersion().onOrAfter(ResizeAction.COMPATIBILITY_VERSION)) {
out.writeEnum(type);
}
if (out.getVersion().onOrAfter(Version.V_6_4_0)) {
out.writeBoolean(copySettings);
if (out.getVersion().before(Version.V_6_4_0)) {

} else if (out.getVersion().onOrAfter(Version.V_6_4_0) && out.getVersion().before(Version.V_7_0_0_alpha1)) {
out.writeBoolean(copySettings == null ? false : copySettings);
} else {
out.writeOptionalBoolean(copySettings);
}
}

Expand Down Expand Up @@ -187,11 +196,11 @@ public ResizeType getResizeType() {
return type;
}

public void setCopySettings(final boolean copySettings) {
public void setCopySettings(final Boolean copySettings) {
this.copySettings = copySettings;
}

public boolean getCopySettings() {
public Boolean getCopySettings() {
return copySettings;
}

Expand Down
Loading