Skip to content

[Rest Api Compatibility] _time and _term sort orders#74919

Merged
pgomulka merged 2 commits intoelastic:masterfrom
pgomulka:compat/_type_time_aggregation
Jul 6, 2021
Merged

[Rest Api Compatibility] _time and _term sort orders#74919
pgomulka merged 2 commits intoelastic:masterfrom
pgomulka:compat/_type_time_aggregation

Conversation

@pgomulka
Copy link
Copy Markdown
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.

@pgomulka
[Rest Api Compatibility] _time and _term sort orders
f6afabb 
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
Copy Markdown
Collaborator

elasticmachine commented Jul 5, 2021

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

@pgomulka
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

elasticmachine commented Jul 27, 2021

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

Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pgomulka @elasticmachine @nik9000 @jakelandis