Skip to content

Update TransportVersion to support a new model #131488

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.

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

Merged
merged 36 commits into from
Jul 24, 2025
Merged

Conversation

jdconrad
Copy link
Contributor

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

Copy link

cla-checker-service bot commented Jul 21, 2025

💚 CLA has been signed

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Looks pretty good, mostly minor comments

@@ -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> {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return new TransportVersion(null, id, null);
}

public static TransportVersion fromName(String name) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (onOrAfter(version)) {
return true;
}
TransportVersion nextPatchVersion = version.nextPatchVersion;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

@Override
public String toString() {
return Integer.toString(id);
return "" + id;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

private static Map<String, TransportVersion> loadTransportVersionsByName() {
Map<String, TransportVersion> transportVersions = new HashMap<>();

String latestLocation = "/transport/latest/" + Version.CURRENT.major + "." + Version.CURRENT.minor + "-LATEST.json";
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

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 + "]"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

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";
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -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");
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed any production examples from this specific PR.


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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I removed this.

@elasticsearchmachine
Copy link
Collaborator

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

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

@JVerwolf JVerwolf left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -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> {
Copy link
Member

Choose a reason for hiding this comment

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

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

@jdconrad jdconrad merged commit f952838 into elastic:main Jul 24, 2025
33 checks passed
jdconrad added a commit to jdconrad/elasticsearch that referenced this pull request Jul 24, 2025
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
Copy link
Collaborator

💔 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

jdconrad added a commit to jdconrad/elasticsearch that referenced this pull request Jul 24, 2025
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 a commit to jdconrad/elasticsearch that referenced this pull request Jul 24, 2025
This change updates `TransportVersion` to support our new model while still allowing the old model to
work as well giving us time to migrate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged backport pending :Core/Infra/Transport API Transport client API >refactoring Team:Core/Infra Meta label for core/infra team v8.18.5 v8.19.1 v9.0.5 v9.1.1 v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants