WIP Update TransportVersion to support a new model
#131488
Conversation
jdconrad
added
>non-issue
WIP
:Core/Infra/Transport API
|
💚 CLA has been signed |
| @@ -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> { | |||
There was a problem hiding this comment.
Is there something about these changes that precludes using a record?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You can customize equals and hashCode for the record to only consider the id field.
| return new TransportVersion(null, id, null); | ||
| } | ||
|
|
||
| public static TransportVersion fromName(String name) { |
There was a problem hiding this comment.
Javadocs please, referencing how these are are in resource files and looked up
| if (onOrAfter(version)) { | ||
| return true; | ||
| } | ||
| TransportVersion nextPatchVersion = version.nextPatchVersion; |
There was a problem hiding this comment.
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?
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return Integer.toString(id); | ||
| return "" + id; |
There was a problem hiding this comment.
Why use string building instead of the explicit toString that was here before?
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
fwiw since we already have the "latest" prefix, we don't also need the "-LATEST" suffix
| 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 + "]" |
There was a problem hiding this comment.
nit: this isn't a release version, it's a minor
| 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"; |
There was a problem hiding this comment.
nit: similar to the above, the prefix already has generated, so we don't also need the file to have "generated"
There was a problem hiding this comment.
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"); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I removed any production examples from this specific PR.
|
|
||
| public void testNamedVersions() { | ||
| assertEquals( | ||
| new TransportVersion("esql-split-on-big-values", 9116000, null), |
There was a problem hiding this comment.
Can we have tests that are not dependent on production constants? Otherwise these tests must be iterated on over time
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
This manifest file shouldn't be checked in, right? Otherwise this isn't backportable
There was a problem hiding this comment.
Correct. I removed this.
This change updates
TransportVersionto support our new model while still allowing the old model to work as well giving us time to migrate.