Skip to content

[Transform] add method to collect deprecations from a transform configuration #77565

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
Merged
merged 9 commits into from
Sep 14, 2021
Merged

[Transform] add method to collect deprecations from a transform configuration #77565

merged 9 commits into from
Sep 14, 2021

Conversation

@hendrikmuhs
Copy link

@hendrikmuhs hendrikmuhs commented Sep 10, 2021 •
edited
Loading

add checkForDeprecations to TransformConfig and implement deprecation checks

Notes:

  • I refactored/moved some classes to re-use them in transform, see 1st commit: hendrikmuhs@d2938d9:
    • DeprecationIssue from the deprecations plugin /CC @jakelandis
    • LoggingDeprecationAccumulationHandler from ML (semantic move inside x-pack core)
  • the real changes start at commit 2

Approach:

I am using a similar approach to ML, checkForDeprecations parses the configuration again, but instead of the ordinary deprecation logger re-uses the collector from ML datafeeds.

Todo:

  • integrate into deprecation API (followup PR?)

@hendrikmuhs hendrikmuhs force-pushed the transform-deprecations branch from 2030a64 to 2d280f3 Compare September 13, 2021 08:07
@hendrikmuhs hendrikmuhs marked this pull request as ready for review September 13, 2021 13:38
@elasticmachine elasticmachine added the Team:ML Meta label for the ML team label Sep 13, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 13, 2021

Pinging @elastic/ml-core (Team:ML)

benwtrent
benwtrent approved these changes Sep 13, 2021
View reviewed changes
Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

I am fine with the integration into the deprecation API to be in a follow PR, especially since this PR has a larger refactor with moving things around.

Would be good to jake or another es/data-managment team member to bless this PR.

...plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/QueryConfig.java Outdated
Comment on lines +164 to +175
Copy link
Member

@benwtrent benwtrent Sep 13, 2021

Choose a reason for hiding this comment

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

ok, I like this way better than the way ML is doing it (these list of strings, etc.). Good refactoring I think++

przemekwitek
przemekwitek approved these changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

Just 4 nits

...plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/QueryConfig.java Outdated
Copy link
Contributor

@przemekwitek przemekwitek Sep 14, 2021

Choose a reason for hiding this comment

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

Is this local variable needed at all?

Copy link
Author

@hendrikmuhs hendrikmuhs Sep 14, 2021

Choose a reason for hiding this comment

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

yes, for lenient parsing we only log the exception and query remains null. I could also have 2 return's, but I think 1 exit is better than 2.

@hendrikmuhs hendrikmuhs force-pushed the transform-deprecations branch from dc1e431 to 6b9f757 Compare September 14, 2021 07:56
@masseyke masseyke self-requested a review September 14, 2021 14:35
masseyke
masseyke approved these changes Sep 14, 2021
View reviewed changes
Copy link
Member

@masseyke masseyke left a comment

Choose a reason for hiding this comment

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

Hendrick asked me to review just the part about moving DeprecationIssue to x-pack core. That makes sense to me.

@hendrikmuhs hendrikmuhs merged commit 8d8d546 into elastic:master Sep 14, 2021
@hendrikmuhs hendrikmuhs deleted the transform-deprecations branch September 14, 2021 14:56
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Sep 14, 2021

💔 Backport failed

Status Branch Result
❌ 7.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 77565

@hendrikmuhs hendrikmuhs mentioned this pull request Sep 15, 2021
Merged
hendrikmuhs pushed a commit to hendrikmuhs/elasticsearch that referenced this pull request Sep 15, 2021
[Transform] add method to collect deprecations from a transform confi…
4e26eb2 
…guration (elastic#77565)

add checkForDeprecations to TransformConfig and implement deprecation checks
elasticsearchmachine pushed a commit that referenced this pull request Sep 15, 2021
[Transform] add method to collect deprecations from a transform confi…
a246cc9 
…guration (#77565) (#77752)

add checkForDeprecations to TransformConfig and implement deprecation checks
@hendrikmuhs hendrikmuhs mentioned this pull request Sep 28, 2021
Merged
hendrikmuhs pushed a commit that referenced this pull request Sep 28, 2021
[Transform] fix version check for beta transforms (#78364)
dfd65be 
fix the version check for deprecated beta transforms, this was introduced in #77565, but
contained a typo (7.15 instead of 7.5)
hendrikmuhs pushed a commit to hendrikmuhs/elasticsearch that referenced this pull request Sep 28, 2021
[Transform] fix version check for beta transforms (elastic#78364)
8ed7aef 
fix the version check for deprecated beta transforms, this was introduced in elastic#77565, but
contained a typo (7.15 instead of 7.5)
elasticsearchmachine pushed a commit that referenced this pull request Sep 28, 2021
[7.x] [Transform] fix version check for beta transforms (#78364) (#78374
e24a986 
)

* [Transform] fix version check for beta transforms (#78364)

fix the version check for deprecated beta transforms, this was introduced in #77565, but
contained a typo (7.15 instead of 7.5)

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

Reviewers

@masseyke masseyke masseyke approved these changes

@benwtrent benwtrent benwtrent approved these changes

@przemekwitek przemekwitek przemekwitek approved these changes

Assignees

No one assigned

Labels

>enhancement :ml/Transform Transform Team:ML Meta label for the ML team v7.16.0 v8.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@hendrikmuhs @elasticmachine @elasticsearchmachine @masseyke @benwtrent @przemekwitek @jakelandis