Refresh potential lost connections at query start for _search
#130463
Conversation
| @@ -167,6 +168,8 @@ public class TransportSearchAction extends HandledTransportAction<SearchRequest, | |||
| private final Client client; | |||
| private final UsageService usageService; | |||
| private final boolean collectTelemetry; | |||
| private final boolean alwaysEstablishConnection; | |||
| private static final long FORCE_CONNECT_CLUSTER_DEFAULT_TIMEOUT = 3L; | |||
There was a problem hiding this comment.
Probably should add SECS or SECONDS to the name to make it clear what the unit of time is.
There was a problem hiding this comment.
That's a good idea.
| @@ -215,6 +218,8 @@ public TransportSearchAction( | |||
| this.searchResponseMetrics = searchResponseMetrics; | |||
| this.client = client; | |||
| this.usageService = usageService; | |||
| alwaysEstablishConnection = settings.getAsBoolean("serverless.force_cluster_reconnect", false); | |||
There was a problem hiding this comment.
@n1v0lg What do you recommend we do for CPS specific settings like this? Should we all use a common setting (e.g., is_serverless, is_cross_project_enabled) or should we proliferate new settings on case-by-case basis?
There was a problem hiding this comment.
Since this is a CPS specific behaviour, I'd like to rely on a single common setting. The reason why I introduced a new one is because it's inline with our initial thought — introduce a new setting and then perhaps squash all of them into a single common one. But I see no reason why we cannot do it right from the beginning.
| @@ -215,6 +218,8 @@ public TransportSearchAction( | |||
| this.searchResponseMetrics = searchResponseMetrics; | |||
| this.client = client; | |||
| this.usageService = usageService; | |||
| alwaysEstablishConnection = settings.getAsBoolean("serverless.force_cluster_reconnect", false); | |||
| logger.info("Should force reconnect cluster: {}", alwaysEstablishConnection); | |||
There was a problem hiding this comment.
Maybe only log this if it is true? Not sure we need it. Let me know why you think it would be useful.
There was a problem hiding this comment.
Ah, this is just a remnant. I can get rid of it.
For S2D9, we'd need
_searchto refresh potentially lost connections at start: by explicitly establishing a connection with a short timeout to avoid waiting for large duration. This PR is not fully feature complete. Instead, it is to understand if we're on the right track with respect to the preferred solution for the differential behaviour since that meeting hasn't happened yet. Once we're confident on this, I'd then like to finish off the rest of the formalities.Some preliminary questions that we haven't covered so far: