Skip to content

Conversation

@pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Jul 5, 2021

Previously removed in #39450, deprecated in es6 but not removed in es 7.
defaults to _key behaviour

relates #51816

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

Previously removed in elastic#39450, deprecated in es6 but not removed in es 7.
defaults to _key behaviour

relates elastic#51816
@pgomulka pgomulka self-assigned this Jul 5, 2021
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 5, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@pgomulka
Copy link
Contributor Author

pgomulka commented Jul 5, 2021

removal of _time and _term was marked as discuss for rest api compatibility. Hence the team-discuss label
@elastic/es-analytics-geo let me know if we plan to merge the rest api compatible change or backport the removal to 7.x
I would prefer to merge this PR to master and remove the support to _key and _time in master only. It would be easier for users.

@nik9000
Copy link
Member

nik9000 commented Jul 6, 2021

I would prefer to merge this PR to master and remove the support to _key and _time in master only. It would be easier for users.

Do you mean you'd prefer to just merge this to master, no backports? In other words, this removal gets a super long deprecation cycle? That's fine with me. I don't think it's hurting anyone.

@pgomulka
Copy link
Contributor Author

pgomulka commented Jul 6, 2021

Do you mean you'd prefer to just merge this to master, no backports? In other words, this removal gets a super long deprecation cycle?

yes, there is a chance to make it easier for users to upgrade, so why not :)
unless there are other reasons to remove it in 7 which I am not aware of.

@nik9000
Copy link
Member

nik9000 commented Jul 6, 2021

Yeah - I'm 👍 to do it the way you've proposed. Do we have a way of knowing when to drop support for it? I guess at some point we'll remove the constant you are comparing to and just zap all of the statements it guards.

@pgomulka
Copy link
Contributor Author

pgomulka commented Jul 6, 2021

when we bump RestApiVersion.minimumSupported to 8 (current version would be 9) then the code https://github.com/elastic/elasticsearch/pull/74919/files#diff-ab996c9a4e31c3c2e1c2df8238d258c0217fecc258066faa02e0c7a5701255afR578 will become dead.
We will likely follow up after a version bump with a cleanup and remove all usages of RestApiVersion.V_7

@nik9000
Copy link
Member

nik9000 commented Jul 6, 2021

We will likely follow up after a version bump with a cleanup and remove all usages of RestApiVersion.V_7

That's awesome! I that we can remove the dead code in as many changes as we need after dropping the support.

@pgomulka pgomulka merged commit 72a58bc into elastic:master Jul 6, 2021
@jakelandis jakelandis added v8.0.0-alpha1 :Core/Infra/REST API REST infrastructure and utilities and removed v8.0.0 labels Jul 26, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 27, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations :Core/Infra/REST API REST infrastructure and utilities >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Core/Infra Meta label for core/infra team team-discuss v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants