-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Introduce simple remote connection strategy #47480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce simple remote connection strategy #47480
Conversation
Pinging @elastic/es-distributed (:Distributed/Network) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I've left some suggestions for code simplification
server/src/main/java/org/elasticsearch/transport/SimpleConnectionStrategy.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/SimpleConnectionStrategy.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/SimpleConnectionStrategy.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/SimpleConnectionStrategy.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/transport/SimpleConnectionStrategy.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/transport/SimpleConnectionStrategyTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/transport/SimpleConnectionStrategyTests.java
Outdated
Show resolved
Hide resolved
@ywelsch i updated this PR for your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some nits, otherwise looking good.
@@ -161,4 +166,19 @@ boolean assertNoRunningConnections() { | |||
} | |||
return result; | |||
} | |||
|
|||
static Predicate<ClusterName> getRemoteClusterNamePredicate(ClusterName remoteClusterName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only used by SniffConnectionStrategy now. Move it back down?
|
||
private void performSimpleConnectionProcess(Iterator<Supplier<TransportAddress>> addressIter, ActionListener<Void> listener) { | ||
openConnections(listener, 1); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra newline
} else { | ||
int openConnections = connectionManager.size(); | ||
if (openConnections == 0) { | ||
finished.onFailure(new IllegalStateException("Unable to open any simple connections to remote cluster")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention remote cluster name here?
if (openConnections == 0) { | ||
finished.onFailure(new IllegalStateException("Unable to open any simple connections to remote cluster")); | ||
} else { | ||
logger.trace("unable to open maximum number of connections [opened: {}, maximum: {}]", openConnections, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention remote cluster name here, and log at debug level?
trace is for expected things, debug for more unusual stuff
|
||
@Override | ||
public void onFailure(Exception e) { | ||
logger.debug(() -> new ParameterizedMessage("failed to open remote connection to address {}", address), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention cluster name here
return resolvedAddresses.get(Math.floorMod(curr, resolvedAddresses.size())); | ||
} | ||
|
||
private ConnectionManager.ConnectionValidator clusterNameValidator(DiscoveryNode node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove the node
parameter here and have a single instance of the ConnectionValidator
initialized in the constructor. The node here is not that useful, and you can get it from the connection (newConnection.getNode()
).
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.
* 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.
This is great feature, thank you. |
@fpytloun The meta issue is here #49067. As you can see, the work is scoped for 7.6 although things can always change. Currently the "simple" work is currently available on either 7.6 or 8.0 snapshot builds. It can be configured:
Please note that it is still in active development. For example, we are still debating whether we want to name it "simple" or "proxy" (the primary advantage of this mode over sniff is for routing through a proxy to unaddressable nodes). You can follow any updates on the meta issue and the documentation will be updated before any release where this feature is GA. |
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.