Make remote setting updates support diff strategies #47891
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tim-Brooks
added
>non-issue
WIP
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed (:Distributed/Network) |
ywelsch
suggested changes
Oct 22, 2019
server/src/main/java/org/elasticsearch/transport/RemoteConnectionStrategy.java
Outdated
Show resolved
Hide resolved
| @@ -111,6 +136,50 @@ public void onFailure(Exception e) { | |||
| } | |||
| } | |||
|
|
|||
| public static void validateSettings(String clusterAlias, Settings settings) { | |||
| if (RemoteClusterAware.LOCAL_CLUSTER_GROUP_KEY.equals(clusterAlias)) { | |||
| throw new IllegalArgumentException("remote clusters must not have the empty string as its key"); | |||
There was a problem hiding this comment.
we should do this validation in the settings infrastructure, not when applying the settings.
There was a problem hiding this comment.
I agree. I have pulled out most of this code and left it with the same behavior as before. I had looked into the settings validation infrastructure enough to determine that I thought it should be a piece of work on its own (validating that each mode only allows certain settings.)
There was a problem hiding this comment.
ok, can you add a TODO in the code and follow-up with a PR? Thank you.
server/src/main/java/org/elasticsearch/transport/RemoteConnectionStrategy.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/RemoteClusterConnection.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/transport/RemoteClusterConnectionTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/transport/RemoteClusterServiceTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/transport/RemoteClusterServiceTests.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/RemoteConnectionStrategy.java
Outdated
Show resolved
Hide resolved
ywelsch
approved these changes
Oct 24, 2019
| // are on the cluster state thread and our custom future implementation will throw an | ||
| // assertion. | ||
| if (latch.await(10, TimeUnit.SECONDS) == false) { | ||
| logger.warn("failed to connect to updated remote cluster {} within {}", clusterAlias, TimeValue.timeValueSeconds(3)); |
There was a problem hiding this comment.
failed to connect to new remote cluster
also note that you're using 3 seconds in the message here 🗡
Tim-Brooks
added a commit
to Tim-Brooks/elasticsearch
that referenced
this pull request
Oct 24, 2019
Currently the entire remote cluster settings infrastructure is designed around the sniff strategy. As we introduce an additional conneciton strategy this infrastructure needs to be modified to support it. This commit modifies the code so that the strategy implementations will tell the service if the connection needs to be torn down and rebuilt. As part of this commit, we will wait 10 seconds for new clusters to connect when they are added through the "update" settings infrastructure.
Tim-Brooks
added a commit
to Tim-Brooks/elasticsearch
that referenced
this pull request
Oct 24, 2019
Currently the entire remote cluster settings infrastructure is designed around the sniff strategy. As we introduce an additional conneciton strategy this infrastructure needs to be modified to support it. This commit modifies the code so that the strategy implementations will tell the service if the connection needs to be torn down and rebuilt. As part of this commit, we will wait 10 seconds for new clusters to connect when they are added through the "update" settings infrastructure.
Tim-Brooks
added a commit
that referenced
this pull request
Oct 25, 2019
* Extract remote "sniffing" to connection strategy (#47253) Currently the connection strategy used by the remote cluster service is implemented as a multi-step sniffing process in the RemoteClusterConnection. We intend to introduce a new connection strategy that will operate in a different manner. This commit extracts the sniffing logic to a dedicated strategy class. Additionally, it implements dedicated tests for this class. Additionally, in previous commits we moved away from a world where the remote cluster connection was mutable. Instead, when setting updates are made, the connection is torn down and rebuilt. We still had methods and tests hanging around for the mutable behavior. This commit removes those. * Introduce simple remote connection strategy (#47480) This commit introduces a simple remote connection strategy which will open remote connections to a configurable list of user supplied addresses. These addresses can be remote Elasticsearch nodes or intermediate proxies. We will perform normal clustername and version validation, but otherwise rely on the remote cluster to route requests to the appropriate remote node. * Make remote setting updates support diff strategies (#47891) Currently the entire remote cluster settings infrastructure is designed around the sniff strategy. As we introduce an additional conneciton strategy this infrastructure needs to be modified to support it. This commit modifies the code so that the strategy implementations will tell the service if the connection needs to be torn down and rebuilt. As part of this commit, we will wait 10 seconds for new clusters to connect when they are added through the "update" settings infrastructure. * Make remote setting updates support diff strategies (#47891) Currently the entire remote cluster settings infrastructure is designed around the sniff strategy. As we introduce an additional conneciton strategy this infrastructure needs to be modified to support it. This commit modifies the code so that the strategy implementations will tell the service if the connection needs to be torn down and rebuilt. As part of this commit, we will wait 10 seconds for new clusters to connect when they are added through the "update" settings infrastructure.
16 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the entire remote cluster settings infrastructure is designed
around the sniff strategy. As we introduce an additional conneciton
strategy this infrastructure needs to be modified to support it. This
commit modifies the code so that the strategy implementations will tell
the service if the connection needs to be torn down and rebuilt.
As part of this commit, we will wait 10 seconds for new clusters to
connect when they are added through the "update" settings
infrastructure.