Skip to content

Migrate JoinValidationService to TransportVersion#102372

Merged
DaveCTurner merged 1 commit intoelastic:mainfrom
DaveCTurner:2023/11/20/JoinValidation-TransportVersion
Nov 20, 2023
Merged

Migrate JoinValidationService to TransportVersion#102372
DaveCTurner merged 1 commit intoelastic:mainfrom
DaveCTurner:2023/11/20/JoinValidation-TransportVersion

Conversation

@DaveCTurner
Copy link
Copy Markdown
Member

We introduced a new join validation protocol in version 8.3, effectively
a different transport protocol. However today we are still checking the
node's release version when deciding which validation protocol to use.
This commit migrates to using the TransportVersion of the relevant
connection.

We introduced a new join validation protocol in version 8.3, effectively
a different transport protocol. However today we are still checking the
node's release version when deciding which validation protocol to use.
This commit migrates to using the `TransportVersion` of the relevant
connection.
@DaveCTurner DaveCTurner added >non-issue :Core/Infra/Core Core issues without another label v8.12.0 labels Nov 20, 2023
@DaveCTurner DaveCTurner requested a review from thecoop November 20, 2023 11:04
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Nov 20, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
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

@DaveCTurner DaveCTurner merged commit 951acf2 into elastic:main Nov 20, 2023
@DaveCTurner DaveCTurner deleted the 2023/11/20/JoinValidation-TransportVersion branch November 20, 2023 14:51
Copy link
Copy Markdown
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

Looks good but I have a minor comment/question

// This node isn't in the cluster yet so ClusterState#getMinTransportVersion() doesn't apply, we must obtain a specific connection
// so we can check its transport version to decide how to proceed.

final Transport.Connection connection;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: who is going to close the connection?
The behaviour should be the same as before, as the connection was opened but not closed in the original location in doRun as well, so I suppose it's not a problem.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We are not opening the connection here, just getting a connection that someone else is responsible for opening. Its lifecycle is managed here:

transportService.connectToNode(joinRequest.getSourceNode(), new ActionListener<>() {
@Override
public void onResponse(Releasable response) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the code pointer!
Yeah, I should have said "obtained", not "opened" :)

@DaveCTurner DaveCTurner restored the 2023/11/20/JoinValidation-TransportVersion branch June 17, 2024 06:17
arteam added a commit to arteam/elasticsearch that referenced this pull request Oct 11, 2024
We introduced a new join validation protocol in elastic#102372 (8.3), the
legacy protocol can be removed in 9.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.12.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants