TransportVersionUtils#randomVersionBetween does not work with version extensions (106119)

[alexey-ivanov-es]

Introduces a new extension method to VersionExtension enabling extensions to provide additional versions and creates method TransportVersion.getAllVersions returning all transport versions defined by Elasticsearch and the extension. This ensures that TransportVersion.current() always returns the correct current (latest) transport version even if it is defined by an extension.

Fixes #106119

[elasticsearchmachine]

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

[ldematte]

Technically this fixes the bug, but the purpose of the function is to test (randomly) any possible version in a range. For serverless, that means more versions (potentially) Shouldn't we add the versions from the extensions instead of just adjusting the index?

[alexey-ivanov-es]

@ldematte The list of versions is currently only available in a static context. Here are the options I see for making it extendable:

1. Create a service to manage all known versions, which would involve a significant refactoring
2. Take a similar approach to org.elasticsearch.TransportVersion#current - introduce a new method in org.elasticsearch.internal.VersionExtension to return all versions, call it from static context and merge all versions into a single list

I feel that existing approach to version extensions is error-prone, but as we only need the list of all versions for unit tests now, I’m not sure it’s worth the effort to fix it properly. What do you think? Am I missing something?

[ldematte]

I was thinking about 2: using the current extension mechanism, extending it if needed.
You are right in saying that we have to decide if it's worth it or not. Probably ... no. Which means I'm fine with the current code.
But I feel we could ask for a second opinion. @thecoop, wdyt?

[thecoop]

Could we have a ServerlessTransportVersionUtils that explicitly knows about serverless things? Then in the main ES one we can throw if the current version is not a main version, so serverless uses of that class are pointed towards the serverless-specific class.

There's also a question about which versions are actually 'live', which requires knowing about deployed serverless versions. Maybe we could use a hard limit - go back a maximum of 50 versions?

[ldematte]

++ for ServerlessTransportVersionUtils. It's the idea I like the most.

[ldematte]

There is still the problem of unchanged tests running on the serverless codebase; even when we introduce ServerlessTransportVersionUtils, I think the change in this PR would be still needed.
Or should we force separate tests?

[alexey-ivanov-es]

Could we have a ServerlessTransportVersionUtils that explicitly knows about serverless things?

Does this solve the original problem? It seems to me that if core tests using TransportVersionUtils#randomVersionBetween are executed as part of the serverless build, then TransportVersion#current might still return a version that doesn’t exist in core

There's also a question about which versions are actually 'live', which requires knowing about deployed serverless versions. Maybe we could use a hard limit - go back a maximum of 50 versions?

I didn’t quite understand this part. Could you clarify where this question comes up and in what context we would need to go back?

[thecoop]

The issue is that TransportVersionUtils.versionsBetween statically references ALL_VERSIONS, which does not include serverless. So there are two options:

1. Include the set of all versions in the data injected by VersionExtension, and use that.
2. Have a separate ServerlessTransportVersionUtils class that statically knows about serverless transport versions, and blow up if this TransportVersionUtils is used from serverless.

1 is perhaps the more dev-friendly option, and adding another method to VersionExtension is not a bad thing.
2 doesn't need any cross-repo changes between the two.

Additionally, we can think more about what transport versions is likely to be used in serverless whilst doing this change. Because we're always releasing off main, we don't need to test 8.1, 8.2, etc in the serverless repo. Using methods 1 or 2, we can limit the transport versions that are returned by any methods in TransportVersionUtils or ServerlessTransportVersionUtils to those that are likely to be hit in prod serverless - which are only the versions added in the last ~4 weeks. Because we don't have timing information in the source code, we can just use the version numbers themselves, with a number that is likely to include all changes added in the last month, at the average rate of version number additions (I plucked 50 out of thin air)

[thecoop]

LGTM, if @rjernst is also ok with it.

This does mean that TransportVersions.VERSION_IDS will contain constants not defined in TransportVersions, but I think thats ok given the situations that will happen in.

[rjernst]

This does mean that TransportVersions.VERSION_IDS will contain constants not defined in TransportVersions

I don't think we should do that. TransportVersions are supposed to be the versions defined in server, and ServerlessTransportVersions are those defined in serverless. We should keep them separate.

Can we move the "getl all transport versions" on to TransportVersion? That class is supposed to be what handles talking with the extension, which is why the current holder is there (and I think it should remain there).

[alexey-ivanov-es]

I don't think we should do that. TransportVersions are supposed to be the versions defined in server, and ServerlessTransportVersions are those defined in serverless. We should keep them separate.

What is the purpose of having a list of transport versions in a serverless app which doesn't include serverless versions?

Can we move the "getl all transport versions" on to TransportVersion?

I think yes. To me, TransportVersion doesn’t sound like the right place for a method getAllVersions(), but I’m not strongly opposed to it either

[rjernst]

There's some history here. TransportVersion is the "thing" that we want. It has the current() method, and used to have an "all" type method. The constants all used to exist on TransportVersion.

We created TransportVersions to avoid static initialization problems. The constants are defined separately from the type of the constants. ServerlessTransportVersions is then the serverless equivalent, where those constants are defined.

The reason I think the "all" type method belongs on TransportVersion is because that is where current() lives. It is the pivot point that knows how to (in a way that won't break class init) get all the version constants. TransportVersions and ServlerlessTransportVersions should be simple containers of constants. That's not exactly the case right now, but I think we should move back toward that direction.

[alexey-ivanov-es]

Moved getAllVersions to TransportVersion

[rjernst]

This looks pretty good. I have just a couple more thoughts.

[rjernst]

A couple last nits, but otherwise looks good.

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	8.x