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.

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

Undocumented(?) change in default for wait_for_active_shards on close index requests #66419

Closed
DaveCTurner opened this issue Dec 15, 2020 · 6 comments
Closed

Undocumented(?) change in default for wait_for_active_shards on close index requests #66419

DaveCTurner opened this issue Dec 15, 2020 · 6 comments
Assignees
@DaveCTurner
Labels
blocker >bug :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.0.0-alpha1

Comments

@DaveCTurner
Copy link
Contributor

In 7.x the default for ?wait_for_active_shards on a POST /index/_close request is NONE (i.e. do not wait) whereas in 8.0 it defaults to DEFAULT (i.e. whatever the index metadata says). I took a quick look at the breaking changes docs for 8.0 and didn't see this mentioned, and nor do I see anywhere that we're emitting deprecation logs indicating the change in behaviour.

This caught me out when writing some tests (see #66415). Should we consider this a breaking change and warn on cases where folk are relying on it? Perhaps not, it might not break anything. I'm opening this issue to remind us to discuss it at some point before releasing 8.0.

@tlrx WDYT?
@DaveCTurner DaveCTurner added >bug :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. labels Dec 15, 2020
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Dec 15, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@DaveCTurner DaveCTurner mentioned this issue Dec 16, 2020
Merged
@tlrx
Copy link
Member

tlrx commented Dec 17, 2020

Looking at the list of changes when the replication of closed indices was merged in master (#39499) and more precisely at #38854 I see nothing that documents the NONE to DEFAULT change so I think I missed to do it. When the change was merged in 7.2 we decided to use NONE to keep the existing behavior of immediate closing (before 7.2 closing indices was also a way to get rid of unassignable indices...) but it should have been documented as a change for 8.0.

So I agree with you and I think the NONE to DEFAULT move should be documented as breaking in 8.0.

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Dec 17, 2020
@DaveCTurner
Deprecate lack of waiting on shards on index close
a97b644 
In 8.x the default for `?wait_for_active_shards` will change from `NONE`
to `DEFAULT` on calls to `POST /index/_close`. This commit adds a
deprecation warning in 7.x if this parameter is not specified to
encourage users to adopt the new behaviour before upgrading.

Closes elastic#66419
@DaveCTurner DaveCTurner mentioned this issue Dec 17, 2020
Closed
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Dec 17, 2020
@DaveCTurner
Document new waiting on shards on index close
612b7b1 
In 8.x the default for `?wait_for_active_shards` changes from `NONE` to
`DEFAULT` on calls to `POST /index/_close`. This commit adds this change
to the breaking changes docs.

Relates elastic#66419, elastic#66542
@DaveCTurner DaveCTurner mentioned this issue Dec 17, 2020
Merged
@DaveCTurner
Copy link
Contributor Author

Ok 👍 I opened #66543 for the breaking changes doc and #66542 to add deprecation warnings.

DaveCTurner added a commit that referenced this issue Dec 17, 2020
@DaveCTurner
Document new waiting on shards on index close (#66543)
1c059e7 
In 8.x the default for `?wait_for_active_shards` changes from `NONE` to
`DEFAULT` on calls to `POST /index/_close`. This commit adds this change
to the breaking changes docs.

Relates #66419, #66542
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Dec 18, 2020
@DaveCTurner
Add ?wait_for_active_shards=default
169f49f 
Today you cannot explicitly indicate that an operation should use the
usual behaviour of waiting for active shards according to the underlying
index setting. This is a problem for the close index API which has a
default of `none` in 7.x for BWC reasons (see elastic#33888), but the usual
behaviour in 8.0: you cannot today opt-in to the 8.0 behaviour with this
parameter.

This commit adds support for the literal value `default` for the
`wait_for_active_shards` query parameter.

Relates elastic#66419
@DaveCTurner DaveCTurner mentioned this issue Dec 18, 2020
Closed
@DaveCTurner
Copy link
Contributor Author

I think it was JFK that once said

We do these things not because they are easy, but because we thought they would be easy.

It turns out we have to add a new value to the ?wait_for_active_shards parameter to indicate that you want the default behaviour. I opened #66575 to do that.

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Jan 7, 2021
@DaveCTurner
Introduce ?wait_for_active_shards=index-setting
05faaae 
In 7.x the close indices API defaulted to `?wait_for_active_shards=0`
but from 8.0 it defaults to respecting the index settings instead.  This
commit introduces the `index-setting` value for this parameter on this
API allowing users to opt-in to the future behaviour today, and emits a
deprecation warning indicating that the default no longer needs to be
used and will be unsupported in future.

In 7.x a follow up PR will introduce support for the same
`index-setting` value for this parameter and will emit deprecation
warnings if users try and use the default instead.

Relates elastic#66419
@DaveCTurner DaveCTurner mentioned this issue Jan 7, 2021
Merged
DaveCTurner added a commit that referenced this issue Jan 11, 2021
@DaveCTurner
Introduce ?wait_for_active_shards=index-setting (#67158)
ec08f92 
In 7.x the close indices API defaulted to `?wait_for_active_shards=0`
but from 8.0 it defaults to respecting the index settings instead.  This
commit introduces the `index-setting` value for this parameter on this
API allowing users to opt-in to the future behaviour today, and emits a
deprecation warning indicating that the default no longer needs to be
used and will be unsupported in future.

In 7.x a follow up PR will introduce support for the same
`index-setting` value for this parameter and will emit deprecation
warnings if users try and use the default instead.

Relates #66419
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Jan 11, 2021
@DaveCTurner
Warn of change of default of wait_for_active_shards
e325ce1 
In 7.x the close indices API defaults to `?wait_for_active_shards=0` but
from 8.0 it will default to respecting the index settings instead. This
commit introduces the `index-setting` value for this parameter on this
API allowing users to opt-in to the future behaviour today, and starts
to emit a deprecation warning for users that use the default.

Relates elastic#67158
Closes elastic#66419
@DaveCTurner DaveCTurner mentioned this issue Jan 11, 2021
Merged
DaveCTurner added a commit that referenced this issue Jan 13, 2021
@DaveCTurner
Warn of change of default of wait_for_active_shards (#67246)
8660e8d 
In 7.x the close indices API defaults to `?wait_for_active_shards=0` but
from 8.0 it will default to respecting the index settings instead. This
commit introduces the `index-setting` value for this parameter on this
API allowing users to opt-in to the future behaviour today, and starts
to emit a deprecation warning for users that use the default.

Relates #67158
Closes #66419
@DaveCTurner
Copy link
Contributor Author

This is still open because #67246 was reverted, see #67246 (comment).

@DaveCTurner DaveCTurner self-assigned this Jan 13, 2021
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Jan 14, 2021
@DaveCTurner
Permit wait_for_active_shards warnings in master
726450e 
Part of the fixes for elastic#66419, this commit permits nodes to emit the
deprecation warning regarding not specifying `?wait_for_active_shards`
when closing an index in 7.x versions for x ≥ 12. This change is
required on `master` too since the BWC tests encounter these warnings.

Relates elastic#67246, which is the 7.x part of this change.
@DaveCTurner DaveCTurner mentioned this issue Jan 14, 2021
Merged
DaveCTurner added a commit that referenced this issue Jan 14, 2021
@DaveCTurner
Permit wait_for_active_shards warnings in master (#67498)
bc1f50c 
Part of the fixes for #66419, this commit permits nodes to emit the
deprecation warning regarding not specifying `?wait_for_active_shards`
when closing an index in 7.x versions for x ≥ 12. This change is
required on `master` too since the BWC tests encounter these warnings.

Relates #67246, which is the 7.x part of this change.
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Jan 14, 2021
@DaveCTurner
Warn of change of default of wait_for_active_shards
e3ffdfe 
In 7.x the close indices API defaults to `?wait_for_active_shards=0` but
from 8.0 it will default to respecting the index settings instead. This
commit introduces the `index-setting` value for this parameter on this
API allowing users to opt-in to the future behaviour today, and starts
to emit a deprecation warning for users that use the default.

Relates elastic#67158
Retry of elastic#67246 now that elastic#67498 is merged to `master`
Closes elastic#66419
@DaveCTurner DaveCTurner mentioned this issue Jan 14, 2021
Merged
DaveCTurner added a commit that referenced this issue Jan 14, 2021
@DaveCTurner
Warn of change of default of wait_for_active_shards (#67527)
64de0ab 
In 7.x the close indices API defaults to `?wait_for_active_shards=0` but
from 8.0 it will default to respecting the index settings instead. This
commit introduces the `index-setting` value for this parameter on this
API allowing users to opt-in to the future behaviour today, and starts
to emit a deprecation warning for users that use the default.

Relates #67158
Retry of #67246 now that #67498 is merged to `master`
Closes #66419
@DaveCTurner
Copy link
Contributor Author

Closed by #67527

fixmebot bot referenced this issue in VectorXz/elasticsearch Apr 22, 2021
@VectorXz
Create TestFixMe.md
a9fae03
fixmebot bot referenced this issue in VectorXz/elasticsearch May 28, 2021
@VectorXz
Create Helloworld.md
1398a04
fixmebot bot referenced this issue in VectorXz/elasticsearch Aug 4, 2021
@VectorXz
Update Helloworld.md
f68abab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DaveCTurner DaveCTurner

Labels
blocker >bug :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tlrx @jakelandis @DaveCTurner @elasticmachine