Extract remote "sniffing" to connection strategy#47253
Merged
Tim-Brooks merged 16 commits intoelastic:masterfrom Sep 30, 2019
Merged
Extract remote "sniffing" to connection strategy#47253Tim-Brooks merged 16 commits intoelastic:masterfrom
Tim-Brooks merged 16 commits intoelastic:masterfrom
Conversation
Collaborator
|
Pinging @elastic/es-distributed |
ywelsch
reviewed
Sep 30, 2019
Contributor
ywelsch
left a comment
There was a problem hiding this comment.
Looking good. I've left 3 small comments.
server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java
Outdated
Show resolved
Hide resolved
| RemoteConnectionStrategy(ThreadPool threadPool, RemoteConnectionManager connectionManager) { | ||
| this.threadPool = threadPool; | ||
| this.connectionManager = connectionManager; | ||
| connectionManager.getConnectionManager().addListener(this); |
Contributor
There was a problem hiding this comment.
are we not doing this already in the constructor of RemoteClusterConnection?
Contributor
There was a problem hiding this comment.
As you're adding the listener, it would be best to remove the listener in this same class, and not in RemoteClusterConnection
server/src/main/java/org/elasticsearch/transport/SniffConnectionStrategy.java
Show resolved
Hide resolved
ywelsch
approved these changes
Sep 30, 2019
Contributor
ywelsch
left a comment
There was a problem hiding this comment.
Left one more critical comment. Once addressed looks good.
| RemoteConnectionStrategy(ThreadPool threadPool, RemoteConnectionManager connectionManager) { | ||
| this.threadPool = threadPool; | ||
| this.connectionManager = connectionManager; | ||
| connectionManager.getConnectionManager().addListener(this); |
Contributor
There was a problem hiding this comment.
As you're adding the listener, it would be best to remove the listener in this same class, and not in RemoteClusterConnection
Tim-Brooks
added a commit
to Tim-Brooks/elasticsearch
that referenced
this pull request
Oct 24, 2019
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.
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
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 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.