Remove any references to org.elasticsearch.core.RestApiVersion#V_7 by JVerwolf · Pull Request #118103 · elastic/elasticsearch

JVerwolf · 2024-12-05T19:23:44Z

This PR removes any references to org.elasticsearch.core.RestApiVersion#V_7.

To save time, I made the requisite deprecations for ML and Data-Management code that were necessary to remove the V_7 code owned by Core-Infra. After approval from Core-Infra, I will then seek review from both of the other teams prior to merging.

elasticsearchmachine · 2024-12-05T21:41:27Z

Hi @JVerwolf, I've created a changelog YAML for you. Note that since this PR is labelled >deprecation, you need to update the changelog YAML to fill out the extended information sections.

elasticsearchmachine · 2024-12-05T21:48:30Z

Hi @JVerwolf, I've updated the changelog YAML for you.

elasticsearchmachine · 2024-12-05T22:06:15Z

Hi @JVerwolf, I've updated the changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

…ent/remove-rest-api-v-7-references

JVerwolf · 2024-12-06T19:20:40Z

    @UpdateForV10(owner = UpdateForV10.Owner.DATA_MANAGEMENT) // Replace with just RestApiVersion.values() when V8 no longer exists
    public static final List<RestApiVersion> REST_API_VERSIONS_POST_V8 = Stream.of(RestApiVersion.values())
-        .filter(v -> v.compareTo(RestApiVersion.V_8) > 0)
+        .filter(v -> v.major > RestApiVersion.V_8.major)


For the Data Management folks, Can you double check this? Specifically, that values > V_8 is the desired behaviour?

I think this was a bug. After removing V_7, this would return an empty list, causing tests to fail. This was previously returning a list containing only V_7 (as the ordinal for V_7 was greater than the ordinal for V_8). I don't think that behaviour was intended.

@PeteGillinElastic can you confirm here?

Yeah, this is my bad. Thanks for catching it. The intention was to include values later than V_8 — but, of course, compareTo on enums compares by the order the values are defined in the enum, which for RestApiVersion is "backwards", so it was actually getting earlier than V_8 (and even that behaviour was fragile as it depends on the declaration order which shouldn't be part of the contract).[*]

Your change looks good. Alternatively, I wonder whether v.matches(RestApiVersion.onOrAfter(RestApiVersion.V_9)) might be even more explicit? Or v != RestApiVersion.V_8, which is equivalent once V_7 goes away?

Aside: It feels like we should discourage the use of compareTo on RestApiVersion, since its behaviour is so counter-intuitive. I was going to suggest that we could override it with an implementation which just delegates to super and @Deprecate it — but it's declared as final on Enum, so that doesn't work. My personal opinion is that it was probably a mistake for Enum to implement Comparable in the first place, but we make use of it way too many times to consider banning it via static analysis.

[*] If, like me, you're wondering why the test passed with this bug: It's because in the code under test we effectively just test for != RestApiVersion.V_8, which seems safe since we knew that V_7 was going to go away before V9 shipped. We've been writing BWC logic this way to ensure that it explicitly references the older version, V_8 in this case, to guarantee that we remember to remove it when V_8 goes away.

Thanks @PeteGillinElastic , I updated this to use v.matches(RestApiVersion.onOrAfter(RestApiVersion.V_9)) as per your suggestion.

I don't see that change pushed here yet... But LGTM anyway.

I'll leave @dakrone to give the overall approval for the DM stuff when he's ready.

JVerwolf · 2024-12-06T19:23:40Z

    // serialized to older nodes where the transport action was a MasterNodeReadAction.
    // TODO: Make this a simple request in a future version where there is no possibility
    // of this request being serialized to another node.
+    @UpdateForV9(owner = UpdateForV9.Owner.MACHINE_LEARNING)


ML Folks: As per the comment above, can this now be updated for V9? If so I'll leave the annotation.

👍 thanks for creating the annotation

elasticsearchmachine · 2024-12-06T19:26:11Z

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

ldematte

Core/Infra part looks good! Good comments/questions for ML and Data-Management; please follow up with them to ensure the behaviour is still correct.

elasticsearchmachine · 2024-12-09T21:16:19Z

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

elasticsearchmachine · 2024-12-09T21:16:19Z

Pinging @elastic/es-data-management (Team:Data Management)

dakrone

I left some comments from the Data Management side

dakrone · 2024-12-09T21:53:32Z

-            if (restApiVersion != RestApiVersion.V_8 && dataMap.containsKey(Metadata.TYPE.getFieldName())) {
-                deprecationLogger.compatibleCritical(
-                    "simulate_pipeline_with_types",
-                    "[types removal] specifying _type in pipeline simulation requests is deprecated"
-                );
-            }


This was intentional, as we need to deprecate this in 9.0 as well. See #116259 for the background. I believe we still need this code.

Should the logic here be restApiVersion.major < RestApiVersion.V_9.major && ... then?

@dakrone I changed this to:

if (restApiVersion != null && restApiVersion.major < RestApiVersion.V_9.major && dataMap.containsKey(Metadata.TYPE.getFieldName())) {

The null check was required to prevent serverless tests from failing (see gradle build scan for details)

Do we even need a version check here, or is the dataMap.containsKey( enough?

WDYT?

I think that logic is slightly different, because we want to deprecate if the restApiVersion is null still, and we don't want deprecations for the v8 API version, but we do want it for all later versions.

So it should be:

if (dataMap.containsKey(Metadata.TYPE.getFieldName()) && (restApiVersion == null || restApiVersion.major > RestApiVersion.V_8.major)) {

"If the map contains the _type key and either the API version is missing or greater than v8 — issue the deprecation warning"

Makes sense, thanks @dakrone. Fixed.

Wait, are you sure we don't want to log this as critically deprecated in V_8? (We are deprecating in V_8, but not removing, right?)

This fails the test I updated to use V_8 from V_7.

This should be:

if( dataMap.containsKey(Metadata.TYPE.getFieldName()) && (restApiVersion == null || restApiVersion.major < RestApiVersion.V_9.major)) {

We're deprecating in 8.18, and not removing until 10.0. Previously we had:

v7 — not supported
v8 — should be deprecated
v9 — should be deprecated

So I think you're right, instead it should just be:

if (dataMap.containsKey(Metadata.TYPE.getFieldName())) {

With an UpdateForV10 that says "drop support for this in 10.0 unless someone is using v9 API compatibility"

Does that sound right to you?

That sounds right to me, thanks!

dakrone · 2024-12-09T21:55:39Z

    @UpdateForV10(owner = UpdateForV10.Owner.DATA_MANAGEMENT) // Replace with just RestApiVersion.values() when V8 no longer exists
    public static final List<RestApiVersion> REST_API_VERSIONS_POST_V8 = Stream.of(RestApiVersion.values())
-        .filter(v -> v.compareTo(RestApiVersion.V_8) > 0)
+        .filter(v -> v.major > RestApiVersion.V_8.major)


@PeteGillinElastic can you confirm here?

dakrone · 2024-12-09T21:56:17Z

        assertThat(e3.getMessage(), containsString("required property is missing"));
    }
-
-    public void testIngestPipelineWithDocumentsWithType() throws Exception {


We'll still need this test as (unfortunately) this is still available in 9.0 but deprecated.

Roger. Given that this test can no longer reference V_7, I'll update it to V_8 and then adjust the logic in server/src/main/java/org/elasticsearch/action/ingest/SimulatePipelineRequest.java as described in this comment. WDYT?

…ent/remove-rest-api-v-7-references

…om:JVerwolf/elasticsearch into enhancement/remove-rest-api-v-7-references

…ent/remove-rest-api-v-7-references

davidkyle

ML changes LGTM. Thanks for the clean up

davidkyle · 2024-12-11T11:16:20Z

    // serialized to older nodes where the transport action was a MasterNodeReadAction.
    // TODO: Make this a simple request in a future version where there is no possibility
    // of this request being serialized to another node.
+    @UpdateForV9(owner = UpdateForV9.Owner.MACHINE_LEARNING)


👍 thanks for creating the annotation

…om:JVerwolf/elasticsearch into enhancement/remove-rest-api-v-7-references

dakrone

LGTM on the Data Management side

…om:JVerwolf/elasticsearch into enhancement/remove-rest-api-v-7-references

leemthompo · 2025-04-11T10:37:16Z

@JVerwolf is this PR relevant to the serverless changelog? [FYI this question is based on 9.0 breaking changes]

JVerwolf · 2025-04-11T12:43:42Z

@leemthompo Thanks for checking. These are internal to ES and don't represent changes that need to be mentioned to users.

elasticsearchmachine added the v9.0.0 label Dec 5, 2024

remove v_7

eea5e77

JVerwolf force-pushed the enhancement/remove-rest-api-v-7-references branch from f9b5491 to eea5e77 Compare December 5, 2024 21:25

JVerwolf added 2 commits December 5, 2024 13:27

Reformat deprecation comment

9fd4f0c

Move deprecation annotation to type source

2b57954

JVerwolf commented Dec 5, 2024

View reviewed changes

Comment thread server/src/main/java/org/elasticsearch/action/ingest/SimulatePipelineRequest.java

JVerwolf commented Dec 5, 2024

View reviewed changes

Comment thread server/src/main/java/org/elasticsearch/ingest/IngestDocument.java Outdated

JVerwolf added >deprecation :Core/Infra/Core Core issues without another label labels Dec 5, 2024

Update docs/changelog/118103.yaml

0fa61bd

JVerwolf requested review from a team and removed request for a team December 5, 2024 21:41

JVerwolf added >enhancement and removed >deprecation labels Dec 5, 2024

JVerwolf added 2 commits December 5, 2024 13:48

Update docs/changelog/118103.yaml

1e26b2f

Update changelog

c44ae1a

JVerwolf added >breaking and removed >enhancement labels Dec 5, 2024

Update docs/changelog/118103.yaml

1d9983d

JVerwolf added 4 commits December 5, 2024 14:55

Update changelog

c9ec238

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

0ad58f3

…ent/remove-rest-api-v-7-references

Fix broken test

68d9c80

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

e28d697

…ent/remove-rest-api-v-7-references

JVerwolf changed the title ~~[WIP] Remove any references to org.elasticsearch.core.RestApiVersion#V_7~~ Dec 6, 2024

JVerwolf commented Dec 6, 2024

View reviewed changes

JVerwolf marked this pull request as ready for review December 6, 2024 19:25

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Dec 6, 2024

ldematte approved these changes Dec 9, 2024

View reviewed changes

Comment thread docs/changelog/118103.yaml Outdated

Update 118103.yaml

e003c4a

JVerwolf added :ml Machine learning :StorageEngine/Data streams Data streams and their lifecycles labels Dec 9, 2024

elasticsearchmachine added Team:Data Management (obsolete) DO NOT USE. This team no longer exists. Team:ML Meta label for the ML team labels Dec 9, 2024

dakrone requested changes Dec 9, 2024

View reviewed changes

JVerwolf added 4 commits December 9, 2024 15:09

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

217a340

…ent/remove-rest-api-v-7-references

Merge branch 'enhancement/remove-rest-api-v-7-references' of github.c…

2022664

…om:JVerwolf/elasticsearch into enhancement/remove-rest-api-v-7-references

PR Feedback: keep _type

264d168

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

8455509

…ent/remove-rest-api-v-7-references

JVerwolf requested a review from dakrone December 10, 2024 19:04

Add null check

b6d46c6

davidkyle approved these changes Dec 11, 2024

View reviewed changes

JVerwolf and others added 4 commits December 11, 2024 09:27

PR Feedback: Update predicate

0b8cbcb

Merge branch 'main' into enhancement/remove-rest-api-v-7-references

c5d4838

PR Feedback: Update conditional

8573b99

Merge branch 'enhancement/remove-rest-api-v-7-references' of github.c…

779b791

…om:JVerwolf/elasticsearch into enhancement/remove-rest-api-v-7-references

dakrone approved these changes Dec 11, 2024

View reviewed changes

JVerwolf and others added 3 commits December 11, 2024 13:34

Merge branch 'main' into enhancement/remove-rest-api-v-7-references

edd7136

Update conditional

bfed199

Merge branch 'enhancement/remove-rest-api-v-7-references' of github.c…

2e2b876

…om:JVerwolf/elasticsearch into enhancement/remove-rest-api-v-7-references

JVerwolf merged commit a560782 into elastic:main Dec 12, 2024

davidkyle mentioned this pull request Mar 5, 2025

[ML] Remove deprecated routes for ml trained models APIs #124019

Merged

Conversation

JVerwolf commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 5, 2024

elasticsearchmachine commented Dec 5, 2024

elasticsearchmachine commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeteGillinElastic Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JVerwolf Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 6, 2024

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Dec 9, 2024

elasticsearchmachine commented Dec 9, 2024

dakrone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakrone Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JVerwolf Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidkyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakrone left a comment

Choose a reason for hiding this comment

leemthompo commented Apr 11, 2025

JVerwolf commented Apr 11, 2025

Labels

7 participants

JVerwolf commented Dec 5, 2024 •

edited

Loading

PeteGillinElastic Dec 11, 2024 •

edited

Loading

JVerwolf Dec 6, 2024 •

edited

Loading

dakrone Dec 11, 2024 •

edited

Loading

JVerwolf Dec 11, 2024 •

edited

Loading