Skip to content

Expose Connection to remote clusters #113453

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

DaveCTurner
Copy link
Contributor

Sometimes we might need to invoke different requests on a remote cluster
depending on the version of the transport protocol it understands, but
today we cannot make that distinction (without starting to execute an
action on the remote cluster and failing while serializing the request
at least). This commit allows callers access to the underlying
Transport.Connection instance so that we can implement better BwC
logic.

Sometimes we might need to invoke different requests on a remote cluster
depending on the version of the transport protocol it understands, but
today we cannot make that distinction (without starting to execute an
action on the remote cluster and failing while serializing the request
at least). This commit allows callers access to the underlying
`Transport.Connection` instance so that we can implement better BwC
logic.
@DaveCTurner DaveCTurner added >non-issue :Distributed Coordination/Network Http and internode communication implementations v8.16.0 v9.0.0 labels Sep 24, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Sep 24, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@DaveCTurner
Copy link
Contributor Author

Relates #112478: with this change, we can skip over excessively-old remote clusters without needing to rely on throwing exceptions in the request's writeTo method.

@DaveCTurner
Copy link
Contributor Author

Also relates #89456: if we introduce some more efficient requests than today's cluster-state-action-based ones then we need to know what wire protocol the CCR remote understands in order to know whether it will understand those newer requests.

@smalyshev
Copy link
Contributor

This may also help a wider goal of making it easier for CCS to support wider array of older versions (not resolving the issues, but making it a bit easier to address), by making the protocol we're dealing with known earlier in the game. Can't really speak much of the code inside, but I'm +1 on the general idea, and I think it would be useful for #112478.

@DaveCTurner DaveCTurner added the :Core/Infra/Transport API Transport client API label Sep 24, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Sep 24, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@thecoop
Copy link
Member

thecoop commented Sep 25, 2024

Do we need to expose Connection? Can we just expose TransportVersion, or some subset?

@DaveCTurner
Copy link
Contributor Author

Not all that easily - the TransportVersion is only valid in the context of a specific Connection. If we make decisions about the request to send to the remote based on the version then we must use exactly that connection for the request. If the connection closes then we'd rather get an exception back rather than sending the request over a different connection.

We could in theory hide all this behind another interface but meh I'm not sure it's worth it.

Copy link
Contributor

@quux00 quux00 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding this, David.

@DaveCTurner DaveCTurner merged commit 44a5ce3 into elastic:main Sep 26, 2024
15 checks passed
@DaveCTurner DaveCTurner deleted the 2024/09/24/remote-cluster-client-get-connection branch September 26, 2024 07:17
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Sep 26, 2024
Sometimes we might need to invoke different requests on a remote cluster
depending on the version of the transport protocol it understands, but
today we cannot make that distinction (without starting to execute an
action on the remote cluster and failing while serializing the request
at least). This commit allows callers access to the underlying
`Transport.Connection` instance so that we can implement better BwC
logic.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

elasticsearchmachine pushed a commit that referenced this pull request Sep 26, 2024
Sometimes we might need to invoke different requests on a remote cluster
depending on the version of the transport protocol it understands, but
today we cannot make that distinction (without starting to execute an
action on the remote cluster and failing while serializing the request
at least). This commit allows callers access to the underlying
`Transport.Connection` instance so that we can implement better BwC
logic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Transport API Transport client API :Distributed Coordination/Network Http and internode communication implementations >non-issue Team:Core/Infra Meta label for core/infra team Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants