Skip to content

Conversation

JeremyDahlgren
Copy link
Contributor

DisconnectedStrategy.RECONNECT_UNLESS_SKIP_UNAVAILABLE is unsupported in CPS, as skip_unavailable itself is unsupported in CPS. This change adds a check in RemoteClusterService.getRemoteClusterClient() rejecting RECONNECT_UNLESS_SKIP_UNAVAILABLE if stateless is enabled and adds a unit test case for it. This PR is a split from work initially done in #132478.

Resolves: ES-12610

DisconnectedStrategy.RECONNECT_UNLESS_SKIP_UNAVAILABLE is unsupported in CPS,
as skip_unavailable itself is unsupported in CPS.  This change adds a check
in RemoteClusterService.getRemoteClusterClient() rejecting
RECONNECT_UNLESS_SKIP_UNAVAILABLE if stateless is enabled and adds a unit
test case for it.

Resolves: ES-12610
@JeremyDahlgren JeremyDahlgren added >non-issue Team:Distributed Coordination Meta label for Distributed Coordination team :Distributed Coordination/Distributed A catch all label for anything in the Distributed Coordination area. Please avoid if you can. v9.2.0 labels Aug 14, 2025
@JeremyDahlgren JeremyDahlgren requested a review from ywangd August 15, 2025 15:07
@JeremyDahlgren JeremyDahlgren marked this pull request as ready for review August 15, 2025 15:07
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

I wonder whether this change is absolutely necessary. Please correct me if I am wrong: technically it feels OK to use the RECONNECT_UNLESS_SKIP_UNAVAILABLE strategy in stateless? Since stateless has isSkipUnavailable fixed to true all the time, using this strategy means "do not attempt to reconnect". This seems semantically correct to me. Of course if the callers want to reconnect, a different strategy needs to be used and that is up to the caller. So the actual constraint here is the fixed isSkipUnavailable value. If this is well understood by both us and the search side, it seems OK that we can just leave the strategy as is?

Comment on lines +654 to +658
if (isStateless && disconnectedStrategy == DisconnectedStrategy.RECONNECT_UNLESS_SKIP_UNAVAILABLE) {
final var message = "DisconnectedStrategy ["
+ DisconnectedStrategy.RECONNECT_UNLESS_SKIP_UNAVAILABLE
+ "] is not supported in stateless environments";
assert false : message;
Copy link
Member

Choose a reason for hiding this comment

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

We talked about deferring this change until search side has made its changes. But I guess it's OK to proceed here because the condition is guarded with isStateless?

@JeremyDahlgren
Copy link
Contributor Author

I wonder whether this change is absolutely necessary. Please correct me if I am wrong: technically it feels OK to use the RECONNECT_UNLESS_SKIP_UNAVAILABLE strategy in stateless? Since stateless has isSkipUnavailable fixed to true all the time, using this strategy means "do not attempt to reconnect". This seems semantically correct to me. Of course if the callers want to reconnect, a different strategy needs to be used and that is up to the caller. So the actual constraint here is the fixed isSkipUnavailable value. If this is well understood by both us and the search side, it seems OK that we can just leave the strategy as is?

Yes, we can close this out, Pawan is aligned here too, we talked about this when he was working on #132927.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed Coordination/Distributed A catch all label for anything in the Distributed Coordination area. Please avoid if you can. >non-issue Team:Distributed Coordination Meta label for Distributed Coordination team v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants