Refactor usage of compatible version #68648

pgomulka · 2021-02-08T11:28:36Z

Compatible API version is at the moment represented by both Version and
byte - representing a major version. This can lead to a confusion which
representation to use, as well as to incorrect assumptions that minor
versions are supported (with the use of Version.V_7_0_0)

Current usage of XContentParser.useCompatible is also not allowing to
define two compatible implementations. This is not about
support N-2 compatibility, but to allow to continue development when a
major release is performed.

This commit is introducing the CompatibleVersion object responsible for
wrapping around a major version of compatible API.

relates #68100

Compatible API version is at the moment represented by both Version and byte - representing a major version. This can lead to a confusion which representation to use, as well as to incorrect assumptions that minor versions are supperted (with the use of Version.V_7_0_0) Current usage of XContentParser.useCompatible is also not allowing to define two compatibile implementations. This is not about support N-2 compatibility, but to allow to continue development when a major release is performed. This commit is introducing the CompatibleVersion object responsible for wrapping around a major version of compatible API.

elasticmachine · 2021-02-08T11:28:39Z

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

jakelandis

I like the enumeration... as mentioned in the comment, I think it addresses all the concerns I had that motivated the use for a boolean

I think

if([builder|parser|request].getRestApiCompatibleVersion() == RestApiCompatibileVersion.V7) 
{ //

reads pretty nice and no need for the boolean.

Otherwise just a few nit picks.

libs/core/src/main/java/org/elasticsearch/common/compatibility/CompatibleVersion.java

jakelandis · 2021-02-09T00:15:17Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

+        return restCompatibilityVersion;
+    }
+
+    public boolean useCompatibility(CompatibleVersion codeChangeVersion) {


hmm... now seeing this with the new enum, not sure if like the boolean approach.

if(builder.getRestApiCompatibleVersion() == RestApiCompatibileVersion.V7) { //do something special

I think reads pretty well, and looks alot (different enough) from the wire protocol ...its a bit verbose, but such is Java.

I think the enum introduced here resolved any issues I had that led me to suggest the boolean approach.

I agree, I personally prefer to replace boolean expressions with named methods, but in this case it would be an overdo.
I also realised that we can apply the same 'getter' style to parser

jakelandis · 2021-02-09T00:18:11Z

libs/core/src/main/java/org/elasticsearch/common/compatibility/CompatibleVersion.java

+
+    V_8(8),
+    V_7(7),
+    V_6(6);//used in testing, to prove validation


Is there any way to avoid declaring this even for testing ? Having 6 in here, even with the comment is a bit misleading since it will show up in the auto complete.

I agree that this should ideally be removed, but at the same time I won't be able to test the validation against incorrect registration RestController#registerHandlerNoWrap
This is a real scenario that we will have during the major release (3 versions present)

private void registerHandlerNoWrap(RestRequest.Method method, String path, RestHandler maybeWrappedHandler) { final RestApiCompatibleVersion version = maybeWrappedHandler.compatibleWithVersion(); assert RestApiCompatibleVersion.minimumSupported() == version || RestApiCompatibleVersion.currentVersion() == version : "REST API compatibility is only supported for version " + RestApiCompatibleVersion.minimumSupported().major;

my point is that I expect that declaration within Plugin.getRestHandlers of compatible handlers must be done under if statements. If we forget about this the assertion in registerHandlerNoWrap should fail

if(RestApiCompatibleVersion.minimumSupported == RestApiCompatibleVersion.V_8) //register v8 if(RestApiCompatibleVersion.currentVersion == RestApiCompatibleVersion.V_7) //register v7

Even thought those handlers would not be accessible when requested anyway (the validation in RestCompatibleVersionHandler would prevent a request with application/vnd.elasticsearch+json;compatible-with=6 )
I would prefer we don't even try to register them.

jakelandis · 2021-02-09T00:20:14Z

libs/core/src/main/java/org/elasticsearch/common/compatibility/CompatibleVersion.java

+    V_7(7),
+    V_6(6);//used in testing, to prove validation
+
+    // This needs to be aligned with Version.CURRENT


Can this be codified with an assert ? It will trip when cutting the new branch, but here I think that is desirable.

agree, but the assert will have to be in Version class (dependency direction)

jakelandis · 2021-02-09T01:02:29Z

libs/core/src/main/java/org/elasticsearch/common/compatibility/CompatibleVersion.java

+        return valueOf("V_" + majorVersion);
+    }
+
+    public static CompatibleVersion minimumRestCompatibilityVersion() {


nit:

RestApiCompatibleVersion getMinimumSupported()

To avoid duplication in the naming.

jakelandis · 2021-02-09T01:08:45Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

@@ -155,7 +157,7 @@ public static XContentBuilder builder(XContentType xContentType, Set<String> inc
     */
    private boolean humanReadable = false;

-    private byte compatibleMajorVersion;
+    private CompatibleVersion restCompatibilityVersion;


nit: restApiCompatibility

jakelandis · 2021-02-09T01:10:39Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

+    public XContentBuilder withCompatibleVersion(CompatibleVersion compatibleVersion) {
+        assert this.restCompatibilityVersion == null : "Compatible version has already been set";
+        if (compatibleVersion == null) {
+            throw new IllegalArgumentException("Compatible major version must not be null");


nit: the message needs to be updated. Also, can probably use Objects.requireNonNull

jakelandis · 2021-02-09T01:22:32Z

libs/core/src/main/java/org/elasticsearch/common/compatibility/CompatibleVersion.java

+    V_6(6);//used in testing, to prove validation
+
+    // This needs to be aligned with Version.CURRENT
+    public static final CompatibleVersion CURRENT = V_8;


It would be great if we could hide the CURRENT, so a user can't type CompatibleVersion.CURRENT since in all but the core cases (like parsing the header) it would be incorrect to do so.

It's a pretty minor concern ... maybe a simple comment would suffice ? , or maybe a getCurrentVersion() (with java doc saying dont use this) instead to avoid it looking like one of the enum options?

good point, I feel getCurrentVersion can help here.

pgomulka · 2021-02-09T14:41:32Z

@elasticmachine update branch

jakelandis

2 last comments (remove v_6 (and associated test), and boolean method)... assuming that LGTM. I like really like this approach, thanks for the iterations.

jakelandis · 2021-02-09T17:25:19Z

libs/core/src/main/java/org/elasticsearch/common/compatibility/RestApiCompatibleVersion.java

+
+    V_8(8),
+    V_7(7),
+    V_6(6);//used in testing, to prove validation


as discussed in person, we can remove this (V_6) and the associated test, we can have other ways to ensure elder version are not in use, and this would probably cause more confusion (even with the comment) then would help.

agree, removed the instance and the test

jakelandis · 2021-02-09T17:28:05Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

+        return restApiCompatibilityVersion;
+    }
+
+    public boolean useCompatibility(RestApiCompatibleVersion codeChangeVersion) {


should this be removed ?

yes, missed it. thanks

…nto compat/use_compatible_check

jaymode

I went ahead and looked even though its been merged. Looks great!

pgomulka added 2 commits February 8, 2021 11:14

use compatible version in builder

d6db590

pgomulka added :Core/Infra/REST API REST infrastructure and utilities v8.0.0 labels Feb 8, 2021

pgomulka self-assigned this Feb 8, 2021

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Feb 8, 2021

pgomulka added 2 commits February 8, 2021 13:33

fix tests

722e45a

javadoc

211f6ff

pgomulka requested review from jaymode and jakelandis February 8, 2021 17:13

jakelandis reviewed Feb 9, 2021

View reviewed changes

pgomulka added 3 commits February 9, 2021 11:11

parsers to have version instead of flag, review follow up

5de9e93

rename

47ac298

do not use version in rest compatible version helper tests

a1085ba

Merge branch 'master' into compat/use_compatible_check

77e2260

jakelandis approved these changes Feb 9, 2021

View reviewed changes

pgomulka added 2 commits February 10, 2021 08:58

remove unused method and enum instance

ad582de

Merge remote-tracking branch 'pgomulka/compat/use_compatible_check' i…

744918e

…nto compat/use_compatible_check

pgomulka merged commit 71d43b5 into elastic:master Feb 10, 2021

jaymode reviewed Feb 10, 2021

View reviewed changes

pgomulka mentioned this pull request Feb 17, 2021

Consider using boolean flag for compatible API #68100

Closed

pugnascotia added the >refactoring label Mar 31, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

ChrisHegarty unassigned pgomulka Oct 29, 2024

Refactor usage of compatible version #68648

Refactor usage of compatible version #68648

Uh oh!

Conversation

pgomulka commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 8, 2021

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Feb 9, 2021

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pgomulka commented Feb 8, 2021 •

edited

Loading

pgomulka Feb 9, 2021 •

edited

Loading

pgomulka Feb 10, 2021 •

edited

Loading