DesiredNode: deprecate node_version field and make it optional (unused) in current parser#104209
Conversation
|
Hi @ldematte, I've created a changelog YAML for you. Note that since this PR is labelled |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Pinging @elastic/es-distributed (Team:Distributed) |
| storage.writeTo(out); | ||
| Version.writeVersion(version, out); | ||
| if (out.getTransportVersion().onOrAfter(TransportVersions.DESIRED_NODE_VERSION_OPTIONAL_STRING)) { | ||
| out.writeOptionalString(version); |
There was a problem hiding this comment.
Why do we need to maintain the string in the serialization at all?
There was a problem hiding this comment.
Since we are just deprecating the change, I thought it was better to keep the requested value in the response - so if you have a request with version inside (deprecated, but still valid), you find it in the responses too.
This is the most conservative route I think; I'm open to remove it altogether if we agree this is OK.
server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodesTestCase.java
Outdated
Show resolved
Hide resolved
fcofdez
left a comment
There was a problem hiding this comment.
Looks good 👍. I just left a question about the version validation
|
|
||
| private static Version parseLegacyVersion(String version) { | ||
| if (version != null) { | ||
| var semanticVersionMatcher = SEMANTIC_VERSION_PATTERN.matcher(version); |
There was a problem hiding this comment.
Why do we need this check?
There was a problem hiding this comment.
Since the API now accepts anything as node_version, there is change this is an empty string or anything really. An alternative is to have this check during parsing (line 108), so we keep rejecting invalid/non-semantic versions (you either skip it or provide a valid one).
We don't really need it, we just need a Version if we are talking to older nodes (TransportVersion < DESIRED_NODE_VERSION_OPTIONAL_STRING), that's why the check is here.
There was a problem hiding this comment.
But let me know if you prefer to move it in the parser @fcofdez
There was a problem hiding this comment.
I think it's easier to understand if we keep the validation in the parser, but it's not a strong preference 👍
It was deprecated in elastic#104209 (8.13) and shouldn't be set in 9.0
It was deprecated in #104209 (8.13) and shouldn't be set or returned in 9.0 The Desired Nodes API is an internal API, and users shouldn't depend on its backward compatibility.
…ic#114580) It was deprecated in elastic#104209 (8.13) and shouldn't be set or returned in 9.0 The Desired Nodes API is an internal API, and users shouldn't depend on its backward compatibility.
…ic#114580) It was deprecated in elastic#104209 (8.13) and shouldn't be set or returned in 9.0 The Desired Nodes API is an internal API, and users shouldn't depend on its backward compatibility.
…ic#114580) > It was deprecated in elastic#104209 (8.13) and shouldn't be set or returned in 9.0 Note: This PR is still pending on the 9.0 transport version
…lastic#119049) Backports elastic#119049 to 9.0 > Re-submission of elastic#114580 > node_version was deprecated in elastic#104209 (8.13) and shouldn't be set or returned in 9.0
This PR follow our conversation on usage of semantic versions in APIs (new or old).
We agreed that the "no-downgrade" check currently done inside DesiredNode (
DesiredNodesSettingsValidator) is better done externally (e.g. by ECK tools/services). Moving forward, we will need a more refined (and different) way to ensure the new topology is acceptable based on features, not on a catch-all version - the same API in serverless is bound to be quite different.The DesiredNode parser still retains
node_versionto ensure compatibility, but it is now optional (and the parsed valued is discarded) and a deprecation warning is added to the response when this is specified.