-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Conversation
💚 CLA has been signed |
There was a problem hiding this 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> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 + "]" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. I removed this.
Pinging @elastic/es-core-infra (Team:Core/Infra) |
...lClusterTest/java/org/elasticsearch/discovery/ec2/DiscoveryEc2RegularNetworkAddressesIT.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
There was a problem hiding this 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> { |
There was a problem hiding this comment.
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
This change updates `TransportVersion` to support our new model while still allowing the old model to work as well giving us time to migrate.
💔 Backport failed
You can use sqren/backport to manually backport by running |
This change updates `TransportVersion` to support our new model while still allowing the old model to work as well giving us time to migrate.
This change updates `TransportVersion` to support our new model while still allowing the old model to work as well giving us time to migrate.
This change updates
TransportVersion
to support our new model while still allowing the old model to work as well giving us time to migrate.