Remove leniency in LinkedProjectConfig builder methods#139012
Remove leniency in LinkedProjectConfig builder methods#139012JeremyDahlgren merged 11 commits intoelastic:mainfrom
Conversation
Addresses the TODOs in the builders to eliminate support for nulls or empty strings/collections in the builder property setter methods. Resolves: ES-12737
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java
Show resolved
Hide resolved
| if (RemoteClusterSettings.isConnectionEnabled(clusterAlias, mergedSettings)) { | ||
| maybeRebuildConnectionOnCredentialsChange(toConfig(projectId, clusterAlias, mergedSettings), connectionRefs); | ||
| } |
There was a problem hiding this comment.
Two comments:
- Can we have the
RemoteClusterSettings.isConnectionEnabledcheck insidemaybeRebuildConnectionOnCredentialsChange? - Do we need to handle
isConnectionEnabled() == falsescenario?
There was a problem hiding this comment.
I moved the RemoteClusterSettings.isConnectionEnabled() check as suggested, and included it with the other check in the if at the beginning of maybeRebuildConnectionOnCredentialsChange(). The log message is generic enough to cover both scenarios, but I could break them into separate ifs with tailored log messages if you think it would add value?
There was a problem hiding this comment.
Hmm did you push the change? The file TransportReloadRemoteClusterCredentialsAction.java looks the same to me?
There was a problem hiding this comment.
Apologies, I had an issue with my submodule branch. The change should be in cc68a15.
There was a problem hiding this comment.
The log message is fine. I was thinking whether we need to actively call remoteClusterService.remove(...) when isConnectionEnabled() == false. It seems to be a more consistent behaviour with the existing code. That said, the cluster settings updater should tear down the connection if it is not done here. So likely does not matter in practice. I'll leave it up to you to decide.
There was a problem hiding this comment.
I refactored to call remove() if we detect the connection is disabled, 397515c.
ywangd
left a comment
There was a problem hiding this comment.
LGTM
One optional suggestion for actively invoking remoteClusterService.remove(...) for your consideration.
* upstream/main: (79 commits)
Mute org.elasticsearch.test.rest.yaml.CcsCommonYamlTestSuiteIT test {p0=search/140_pre_filter_search_shards/prefilter on non-indexed date fields} elastic#139381
Adjust error bounds for bfloat16 value checks (elastic#139371)
Unmute some vector CSS tests (elastic#139370)
Do not allow `project_routing` as a query param (elastic#139206)
Unmute HalfFloat...Tests#testSynthesizeArrayRandom (elastic#139341)
Remove leniency in LinkedProjectConfig builder methods (elastic#139012)
EQL: fix project_routing (elastic#139366)
Add patch version for 9.2 index version constant (elastic#139362)
Mute org.elasticsearch.test.rest.yaml.RcsCcsCommonYamlTestSuiteIT test {p0=search.vectors/200_dense_vector_docvalue_fields/dense_vector docvalues with bfloat16} elastic#139368
ES|QL: Enable CCS tests for FORK (elastic#139302)
Restructuring the semantic_text field type page (elastic#138571)
AggregateMetricDouble fields should not build BKD indexes (elastic#138724)
Feature/count by trunc with filter (elastic#138765)
ESQL: Convert TS 500 error to 400 (elastic#139360)
[CI] Rerun failing tests for periodic build pipelines (elastic#139200)
revert muting saml test (elastic#139327)
Add TDigest histogram as metric (elastic#139247)
Links solved bugs to class cast exception changelog and unmutes errors (elastic#139340)
Ensure integer sorts are rewritten to long sorts for BWC indexes (elastic#139293)
Integrate stored fields format bloom filter with synthetic _id (elastic#138515)
...
Adds a new unit test class for TransportReloadRemoteClusterCredentialsAction. This is a follow up from elastic#135491, where a significant chunk of code was moved from RemoteClusterService into TransportReloadRemoteClusterCredentialsAction. This also verifies code touched recently in elastic#139012. Resolves: ES-13161
…39414) Adds a new unit test class for TransportReloadRemoteClusterCredentialsAction. This is a follow up from #135491, where a significant chunk of code was moved from RemoteClusterService into TransportReloadRemoteClusterCredentialsAction. This also verifies code touched recently in #139012. Resolves: ES-13161
…astic#139414) Adds a new unit test class for TransportReloadRemoteClusterCredentialsAction. This is a follow up from elastic#135491, where a significant chunk of code was moved from RemoteClusterService into TransportReloadRemoteClusterCredentialsAction. This also verifies code touched recently in elastic#139012. Resolves: ES-13161
Addresses the TODOs in the builders to eliminate support for nulls or empty strings/collections in the builder property setter methods. See this comment from #133266 for details on the origin of the TODOs.
This has two significant API changes:
LinkedProjectConfigListener.remove()LinkedProjectConfig.isConnectionEnabled()It is impossible to construct a disabled LinkedProjectConfig instance since there are explicit checks now in the builders.
All implementations of
LinkedProjectConfigListenerneeded to be updated for the two API changes above.Resolves: ES-12737