Bump compatible rest api version to 9/8 #113151

jakelandis · 2024-09-18T22:36:44Z

This commit bumps the REST API version from 8 to 9. This effectively removes all support for REST API compatibility with version 7 (though earlier commits already chipped away at some v7 support).
This also enables REST API compatibility support for version 8, providing support for v8 compatibility headers, i.e. "application/vnd.elasticsearch+json;compatible-with=8" and no-op support (no errors) to accept v9 compatibility headers i.e. "application/vnd.elasticsearch+json;compatible-with=9".

In addition to 8->9 version bump, the following was also necessary:

Introduce RouteBuilder.deprecateAndKeep to allow us to deprecate an HTTP route/path without explicitly removing it. Prior to this commit, the RouteBuilder.deprecated was ambiguous in what it really meant. Technically, it meant to deprecate that route/path in the expressed Rest API version, and require the compatibility headers in the next version. For example:

Route.builder(POST, "/_foo/{bar}").deprecated(DEPRECATION_WARNING, RestApiVersion.V_8).build(),

Means that _foo/bar is available is v8 and will emit a deprecation warning, but is only accessible in v9 by using the special compatibility header. The usage hints at an expectation that the route would not be removed.

It is not clear if the intent was to simply emit deprecation warnings, or if the intent was to emit deprecation warning AND remove in the next version. To avoid removing many API's by simply bumping the REST API version; this commit introduces RouteBuilder.deprecateAndKeep to allow us to deprecate an HTTP route/path without explicitly removing it. All uses of .deprecated where the version is RestApiVersion.V_8 have been updated to RouteBuilder.deprecateAndKeep with an annotation and comment to revisit if the intent was to actually remove the API or only emit deprecation warnings. This commit will not result in removal of an APIs and allows them to continue to function as they have.

The following classes have been updated and annotated to use RouteBuilder.deprecateAndKeep:

org.elasticsearch.rest.action.search.RestKnnSearchAction
org.elasticsearch.xpack.frozen.rest.action.RestFreezeIndexAction
org.elasticsearch.rest.action.admin.indices.RestPutIndexTemplateAction
org.elasticsearch.xpack.ml.rest.job.RestPostDataAction
org.elasticsearch.xpack.ml.rest.inference.RestDeleteTrainedModelAction
org.elasticsearch.xpack.ml.rest.inference.RestGetTrainedModelsAction
org.elasticsearch.xpack.ml.rest.inference.RestGetTrainedModelsStatsAction
org.elasticsearch.xpack.ml.rest.inference.RestPutTrainedModelAction
org.elasticsearch.xpack.textstructure.rest.RestFindStructureAction
org.elasticsearch.xpack.ml.rest.inference.RestInferTrainedModelDeploymentAction
org.elasticsearch.xpack.ml.rest.inference.RestInferTrainedModelDeploymentAction

Owner's of these classes will need to revisit the code after this commit (may also require updates to tests). cc @elastic/es-data-management @elastic/ml-core

This commit will remove support for elder name of inference_threads/threads_per_allocation and the elder name of model_threads/number_of_allocations HTTP parameters, unless the compatibility header is used. Note - serverless does not use compatibility headers, so the elder name will effectively be removed from serverless. cc: @elastic/ml-core
Removal of any unit test that was testing v7 compatibility. This is needed to get the build to pass, and these tests are no longer relevant.
Skipping indices.create/synthetic_source*.yml compatible REST tests, they will not pass. This is likely due to differences between 8.x and main branches. An internal ticket ES-9597 has been logged to address. cc: @elastic/es-storage-engine
The capabilities API serialization has been hard coded to RestAPI version 8. This serialization does not guard against serializing to elder 8.x clusters that don't understand 9. cc: @elastic/es-core-infra
Any hard coded version of 7 have been updated to 8 and any 8' updated to 9 -> these tests are typically just ensuring we can parse values correctly.
A very weird edge for testing where the routes registered, which change based on the RestAPI version, influence the response codes which can fail tests. Comments added in the source code, but some test clean up accidentally relies on some deep implementation details w.r.t. the response code to determine success or failure of the clean up step.

jakelandis · 2024-09-24T19:56:42Z

server/src/test/java/org/elasticsearch/rest/action/search/RestSearchActionTests.java

-        RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withHeaders(
-            Map.of("Content-Type", contentTypeHeader, "Accept", contentTypeHeader)
-        ).withMethod(RestRequest.Method.GET).withPath("/some_index/_search").withParams(params).build();
+        RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.GET)


these tests look like the explicit headers was just a copy/paste and not relevant to the thing it is actually testing.

elasticsearchmachine · 2024-09-24T20:33:29Z

Pinging @elastic/es-delivery (Team:Delivery)

jakelandis · 2024-09-24T20:33:58Z

@elastic/ml-core - the changes here impact your code base the most. Can you please review the changes to ml and approve this PR.

thecoop · 2024-09-25T08:21:18Z

...g/elasticsearch/action/admin/cluster/node/capabilities/TransportNodesCapabilitiesAction.java

@@ -159,7 +164,9 @@ public void writeTo(StreamOutput out) throws IOException {
            out.writeString(path);
            out.writeCollection(parameters, StreamOutput::writeString);
            out.writeCollection(capabilities, StreamOutput::writeString);
-            out.writeVInt(restApiVersion.major);
+            // Fixme: lies! all lies!
+            out.writeVInt(8);


We need to decide what to do when asked the capabilities of a mixed v8/v9 cluster and a v9 rest api - probably to check the transport versions/available rest API versions beforehand, and answer the check before querying any of the nodes if it cannot be supported (there's several other similar capabilities optimisations to do too).

@thecoop - are you OK with how it is for this PR and address the proper fix later ?

Yes, this can be merged as-is - the @UpdateForV9 will prompt us to fix it later

jan-elastic

The changes to the ml plugin and text_structure plugin LGTM

bump compatible rest api version

93c1166

jakelandis added >non-issue Team:Delivery Meta label for Delivery team v9.0.0 labels Sep 18, 2024

jakelandis mentioned this pull request Sep 18, 2024

Bump the min java version for examples #112843

Merged

jakelandis added 21 commits September 18, 2024 17:43

spotless

bfc81ec

remove explicit reference to 7 from test

b92c182

remove explicit refernce to 7 from test

e3dec13

remove explicit refernce to 7 from test

05ed56e

annonate code to remove that exists only for v7 compat

af6a0b0

remove explicit refernce to 7 from test

fdc2092

update header version test to new version

65f9738

spotless

dd2b705

fix test issue

a6c5ff7

fix ambigous API

0bff2bb

remove test that explicitly tests V7 compatiblity

c759f36

undo deprecrate with removal in v9 for some API's

0d0a425

ml fixes and nits

4c45a65

minor adjustment to test

db33cdf

remove v7 testing from RestActionTestCase

80d97cd

update test

4c8fc78

remove v7 from test

d6a6cee

more test fixes

34aae04

Merge remote-tracking branch 'upstream/main' into bump_compat_api

5c07f81

more ml test fixes

7c8a24c

fix test

ab5d9dc

jakelandis commented Sep 24, 2024

View reviewed changes

jakelandis marked this pull request as ready for review September 24, 2024 20:32

jakelandis requested review from a team as code owners September 24, 2024 20:32

elasticsearchmachine added needs:triage Requires assignment of a team area label and removed Team:Delivery Meta label for Delivery team labels Sep 24, 2024

jakelandis requested a review from a team September 24, 2024 20:32

jakelandis added :Delivery/Build Build or test infrastructure and removed needs:triage Requires assignment of a team area label labels Sep 24, 2024

elasticsearchmachine added the Team:Delivery Meta label for Delivery team label Sep 24, 2024

thecoop reviewed Sep 25, 2024

View reviewed changes

jan-elastic reviewed Sep 26, 2024

View reviewed changes

jan-elastic approved these changes Sep 26, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into bump_compat_api

26372ed

mark-vieira self-requested a review September 26, 2024 19:06

mark-vieira approved these changes Sep 26, 2024

View reviewed changes

jakelandis merged commit 8881886 into elastic:main Sep 26, 2024
15 checks passed

jakelandis mentioned this pull request Sep 30, 2024

Better support for RouteBuilder for deprecate for removal vs. deprecate and keep #113712

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump compatible rest api version to 9/8 #113151

Bump compatible rest api version to 9/8 #113151

jakelandis commented Sep 18, 2024 •

edited

Loading

jakelandis Sep 24, 2024

elasticsearchmachine commented Sep 24, 2024

jakelandis commented Sep 24, 2024

thecoop Sep 25, 2024 •

edited

Loading

jakelandis Sep 25, 2024

thecoop Sep 26, 2024

jan-elastic left a comment

Bump compatible rest api version to 9/8 #113151

Bump compatible rest api version to 9/8 #113151

Conversation

jakelandis commented Sep 18, 2024 • edited Loading

jakelandis Sep 24, 2024

Choose a reason for hiding this comment

elasticsearchmachine commented Sep 24, 2024

jakelandis commented Sep 24, 2024

thecoop Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

jakelandis Sep 25, 2024

Choose a reason for hiding this comment

thecoop Sep 26, 2024

Choose a reason for hiding this comment

jan-elastic left a comment

Choose a reason for hiding this comment

jakelandis commented Sep 18, 2024 •

edited

Loading

thecoop Sep 25, 2024 •

edited

Loading