Update `TransportVersion` to support a new model #131488

jdconrad · 2025-07-17T23:49:35Z

This change updates TransportVersion to support our new model while still allowing the old model to work as well giving us time to migrate.

cla-checker-service · 2025-07-21T20:01:04Z

💚 CLA has been signed

rjernst

Looks pretty good, mostly minor comments

rjernst · 2025-07-21T21:21:16Z

server/src/main/java/org/elasticsearch/TransportVersion.java

@@ -57,7 +69,68 @@
 * different version value. If you need to know whether the cluster as a whole speaks a new enough {@link TransportVersion} to understand a
 * newly-added feature, use {@link org.elasticsearch.cluster.ClusterState#getMinTransportVersion}.
 */
-public record TransportVersion(int id) implements VersionId<TransportVersion> {
+public class TransportVersion implements VersionId<TransportVersion> {


Is there something about these changes that precludes using a record?

I had gone down making the members private without getters in an initial version, but I don't think that makes sense anymore. I switched this back to a record. One thing I wasn't completely sure about is the implications of having name included as part of equals/hashcode. I would hope nothing is using these for direct comparisons for any type of serialization branching.

You can customize equals and hashCode for the record to only consider the id field.

Sure, but is it correct to only use id for this?

I changed this to only use id to maintain exactly what was already there.

rjernst · 2025-07-21T21:22:53Z

server/src/main/java/org/elasticsearch/TransportVersion.java

+        return new TransportVersion(null, id, null);
+    }
+
+    public static TransportVersion fromName(String name) {


Javadocs please, referencing how these are are in resource files and looked up

Javadoc should probably also explain usage/generation, or perhaps leave a TODO untill that part is finalized.

Thinking out loud: fromName is fine, but I'm wondering if we can somehow indicate that this method is special, and unique from fromId? E.g. can we indicate how this method isn't just used to get a TV from a name, but how it also generates/declares the TV too. Some ideas: withName, declare, usingName, access, use, retrieve. TBH I don't know if any of those are better. Hmm, I suppose I'm fine if we stick with this.

I have added JavaDocs for the methods that would be used by consumers. I consider loading to be generally internal.

rjernst · 2025-07-21T21:27:25Z

server/src/main/java/org/elasticsearch/TransportVersion.java

+        if (onOrAfter(version)) {
+            return true;
+        }
+        TransportVersion nextPatchVersion = version.nextPatchVersion;


This linked structure makes the logic a little harder to follow. I'm still not sure it's better than an array of patch ids. But if we are to use it, can we at least document it?

Nit: I can see how this is simpler on one hand, and also creates less objects in memory. However, I'm uncertain about perf implications, if that even matters? This is effectively the same as boxing/wrapping, as our comparison now requires loading objects from the heap, IIUC. I'm not sure that matters, however, so I'll leave it as your call Jack.

I would prefer to leave it this way for now as I think having duplicate ids floating around even if an internal representation may be confusing. Right now it should have similar performance to checking each patch version since those ids are "unboxed" as well.

rjernst · 2025-07-21T21:28:04Z

server/src/main/java/org/elasticsearch/TransportVersion.java

    }

    @Override
    public String toString() {
-        return Integer.toString(id);
+        return "" + id;


Why use string building instead of the explicit toString that was here before?

I added the new members to toString, but later decided to not make changes here. I reverted this to how it was originally.

rjernst · 2025-07-21T23:03:59Z

server/src/main/java/org/elasticsearch/TransportVersion.java

+        private static Map<String, TransportVersion> loadTransportVersionsByName() {
+            Map<String, TransportVersion> transportVersions = new HashMap<>();
+
+            String latestLocation = "/transport/latest/" + Version.CURRENT.major + "." + Version.CURRENT.minor + "-LATEST.json";


fwiw since we already have the "latest" prefix, we don't also need the "-LATEST" suffix

rjernst · 2025-07-21T23:05:05Z

server/src/main/java/org/elasticsearch/TransportVersion.java

+                TransportVersion latest = fromXContent(inputStream, Integer.MAX_VALUE);
+                if (latest == null) {
+                    throw new IllegalStateException(
+                        "invalid latest transport version for release version [" + Version.CURRENT.major + "." + Version.CURRENT.minor + "]"


nit: this isn't a release version, it's a minor

rjernst · 2025-07-21T23:06:16Z

server/src/main/java/org/elasticsearch/TransportVersion.java

-                ALL_VERSIONS = TransportVersions.DEFINED_VERSIONS;
-            } else {
-                ALL_VERSIONS = Stream.concat(TransportVersions.DEFINED_VERSIONS.stream(), extendedVersions.stream()).sorted().toList();
+            String manifestLocation = "/transport/generated/generated-transport-versions-files-manifest.txt";


nit: similar to the above, the prefix already has generated, so we don't also need the file to have "generated"

I changed this to just manifest.txt, and I'm guessing we will have one in both constant and generated folders, but not 100% sure yet.

rjernst · 2025-07-21T23:16:02Z

...search/xpack/inference/services/azureaistudio/rerank/AzureAiStudioRerankServiceSettings.java

@@ -80,7 +79,7 @@ public String getWriteableName() {

    @Override
    public TransportVersion getMinimalSupportedVersion() {
-        return TransportVersions.ML_INFERENCE_AZURE_AI_STUDIO_RERANK_ADDED;
+        return TransportVersion.fromName("ml-inference-azure-ai-studio-rerank");


We should avoid doing a hash lookup every time getMinimalSupportedVersion is called. Ideally we continue to have a common static constant shared with the other settings class this is used in, but at least minimum lets have a constant here if we don't have a common place for such a constant.

I removed any production examples from this specific PR.

rjernst · 2025-07-21T23:16:49Z

server/src/test/java/org/elasticsearch/TransportVersionTests.java

+
+    public void testNamedVersions() {
+        assertEquals(
+            new TransportVersion("esql-split-on-big-values", 9116000, null),


Can we have tests that are not dependent on production constants? Otherwise these tests must be iterated on over time

I updated the testing to have resource files specifically for tests (in test/resource/...) that will exercise the new additions to TransportVersion.

rjernst · 2025-07-21T23:18:38Z

server/src/main/resources/transport/generated/generated-transport-versions-files-manifest.txt

@@ -0,0 +1,3 @@
+esql-split-on-big-values.json


This manifest file shouldn't be checked in, right? Otherwise this isn't backportable

Correct. I removed this.

elasticsearchmachine · 2025-07-23T17:33:19Z

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

...lClusterTest/java/org/elasticsearch/discovery/ec2/DiscoveryEc2RegularNetworkAddressesIT.java

JVerwolf

LGTM.

In the future, I'd like to add a link in the text files and places we throw exceptions that references a markdown document that explains how to resolve conflicts, generate things, etc. But we can hold off for now till we have that machinery.

rjernst

LGTM

rjernst · 2025-07-24T22:06:44Z

server/src/main/java/org/elasticsearch/TransportVersion.java

@@ -57,7 +73,55 @@
 * different version value. If you need to know whether the cluster as a whole speaks a new enough {@link TransportVersion} to understand a
 * newly-added feature, use {@link org.elasticsearch.cluster.ClusterState#getMinTransportVersion}.
 */
-public record TransportVersion(int id) implements VersionId<TransportVersion> {
+public record TransportVersion(String name, int id, TransportVersion nextPatchVersion) implements VersionId<TransportVersion> {


for followup, we should validate this name is lowercase alphanumeric+underscores

This change updates `TransportVersion` to support our new model while still allowing the old model to work as well giving us time to migrate.

elasticsearchmachine · 2025-07-24T22:09:24Z

💔 Backport failed

Status	Branch	Result
✅	9.1
❌	8.19	Commit could not be cherrypicked due to conflicts
✅	9.0
❌	8.18	Commit could not be cherrypicked due to conflicts

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

This change updates `TransportVersion` to support our new model while still allowing the old model to work as well giving us time to migrate.

jdconrad added 4 commits July 17, 2025 15:17

add primary patch version to transport version

340136c

add loading for new transport version model

46cf098

update transport versions holder

a4ec0d6

fix bugs

dc9cfe9

jdconrad requested a review from a team as a code owner July 17, 2025 23:49

jdconrad added >non-issue WIP :Core/Infra/Transport API Transport client API v9.2.0 labels Jul 17, 2025

jdconrad added 4 commits July 18, 2025 08:10

Merge branch 'main' into transport0

a20dfd7

update name

b772a3a

Merge branch 'main' into transport0

3182209

fix equals/hashcode/toString

8aa55ca

spotless

f13e723

jdconrad force-pushed the transport0 branch from 3f374f4 to f13e723 Compare July 21, 2025 20:08

jdconrad added 5 commits July 21, 2025 13:09

Merge branch 'main' into transport0

ead340b

migrate version with multiple patches

be9d70b

spotless

01511ff

fix names

cba38b1

Merge branch 'main' into transport0

f9bebe8

rjernst reviewed Jul 21, 2025

View reviewed changes

jdconrad added 2 commits July 22, 2025 08:09

name fixes

b02cb1a

Merge branch 'main' into transport0

a19caac

jdconrad force-pushed the transport0 branch from ab9463a to 76e3f0d Compare July 22, 2025 19:12

move examples to tests

c56ec3a

jdconrad force-pushed the transport0 branch from 76e3f0d to c56ec3a Compare July 22, 2025 19:13

jdconrad added 3 commits July 22, 2025 12:36

fix new instances

75fbbe4

add versions to test files

c42bfc2

Merge branch 'main' into transport0

1b272b6

jdconrad added >refactoring v9.1.1 v8.19.1 v9.0.5 v8.18.5 and removed WIP labels Jul 23, 2025

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 23, 2025

JVerwolf reviewed Jul 23, 2025

View reviewed changes

...lClusterTest/java/org/elasticsearch/discovery/ec2/DiscoveryEc2RegularNetworkAddressesIT.java Outdated Show resolved Hide resolved

jdconrad added 2 commits July 23, 2025 11:25

fix ij refactoring

eb0a94f

more fixed files

518ed4b

jdconrad added the auto-backport Automatically create backport pull requests when merged label Jul 23, 2025

jdconrad added 6 commits July 23, 2025 11:41

add latest placeholder files for backport versions

14e2539

Merge branch 'main' into transport0

ec39480

switch to loading versions with txt files instead of json

18d2206

revert lazy change

aa7a4bc

Merge branch 'main' into transport0

5da94f9

Merge branch 'main' into transport0

e097c7a

JVerwolf approved these changes Jul 24, 2025

View reviewed changes

update to use csv files and remove names from files when possible

8c8ecfc

rjernst approved these changes Jul 24, 2025

View reviewed changes

jdconrad merged commit f952838 into elastic:main Jul 24, 2025
33 checks passed

This was referenced Jul 24, 2025

[9.1] Update TransportVersion to support a new model (#131488) #131886

Open

[9.0] Update TransportVersion to support a new model (#131488) #131887

Open

elasticsearchmachine added the backport pending label Jul 24, 2025

jdconrad mentioned this pull request Jul 24, 2025

[8.19] Update TransportVersion to support a new model (#131488) #131889

Open

Update TransportVersion to support a new model #131488

Update TransportVersion to support a new model #131488

Conversation

jdconrad commented Jul 17, 2025

Uh oh!

cla-checker-service bot commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjernst left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jul 23, 2025

Uh oh!

Uh oh!

JVerwolf left a comment

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 24, 2025

💔 Backport failed

Uh oh!

Uh oh!

Update `TransportVersion` to support a new model #131488

Update `TransportVersion` to support a new model #131488

cla-checker-service bot commented Jul 21, 2025 •

edited

Loading