Move ConnectionManager to async APIs #42636
Merged
Conversation
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
ywelsch
added
>enhancement
WIP
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed |
ywelsch
requested review from
DaveCTurner and
Tim-Brooks
and removed request for
DaveCTurner
|
@tbrooks8 (transport) @DaveCTurner (cluster coordination) this is ready for review now. |
DaveCTurner
reviewed
Jul 3, 2019
server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/coordination/JoinHelper.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/discovery/HandshakingTransportAddressConnector.java
Show resolved
Hide resolved
test/framework/src/main/java/org/elasticsearch/test/disruption/DisruptableMockTransport.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/cluster/NodeConnectionsServiceTests.java
Show resolved
Hide resolved
Tim-Brooks
reviewed
Jul 4, 2019
| Transport.Connection connection = connectedNodes.get(node); | ||
| if (connection != null) { | ||
| assert connectingNodes.containsKey(node) == false; | ||
| lock.close(); |
There was a problem hiding this comment.
Unnecessary since try/with/resources with release.
There was a problem hiding this comment.
I wanted to release this before calling the response listener.
| return; | ||
| } | ||
|
|
||
| final List<ActionListener<Void>> connectionListeners = connectingNodes.computeIfAbsent(node, n -> new ArrayList()); |
| } finally { | ||
| final Transport.Connection finalConnection = conn; | ||
| conn.addCloseListener(ActionListener.wrap(() -> { | ||
| logger.info("close listener called for node {}", node); |
There was a problem hiding this comment.
I don't totally understand the value of this logging message. Especially since there might be multiple and other close listeners attached to this connection. This message seems to imply THIS is the close listener. It would make more sense to me if the message at lease reflected what this listener is going (deregistering the node and notify that it has disconnected).
There was a problem hiding this comment.
this was a leftover from a debugging session :)
There was a problem hiding this comment.
turned this into trace log at 3a0fc08
DaveCTurner
approved these changes
Jul 4, 2019
ywelsch
added a commit
that referenced
this pull request
Jul 5, 2019
This commit converts the ConnectionManager's openConnection and connectToNode methods to async-style. This will allow us to not block threads anymore when opening connections. This PR also adapts the cluster coordination subsystem to make use of the new async APIs, allowing to remove some hacks in the test infrastructure that had to account for the previous synchronous nature of the connection APIs.
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Jul 8, 2019
Since elastic#42636 we no longer treat connections specially when simulating a blackholed connection. This means that at the end of the safety phase we may have just started a connection attempt which will time out, but the default timeout is 30 seconds, much longer than the 2 seconds we normally allow for post-safety-phase discovery. This commit adds time for such a connection attempt to time out. It also fixes some spurious logging of `this` that now refers to an object with an unhelpful `toString()` implementation introduced in elastic#42636. Fixes elastic#44073
DaveCTurner
added a commit
that referenced
this pull request
Jul 9, 2019
Since #42636 we no longer treat connections specially when simulating a blackholed connection. This means that at the end of the safety phase we may have just started a connection attempt which will time out, but the default timeout is 30 seconds, much longer than the 2 seconds we normally allow for post-safety-phase discovery. This commit adds time for such a connection attempt to time out. It also fixes some spurious logging of `this` that now refers to an object with an unhelpful `toString()` implementation introduced in #42636. Fixes #44073
DaveCTurner
added a commit
that referenced
this pull request
Jul 9, 2019
Since #42636 we no longer treat connections specially when simulating a blackholed connection. This means that at the end of the safety phase we may have just started a connection attempt which will time out, but the default timeout is 30 seconds, much longer than the 2 seconds we normally allow for post-safety-phase discovery. This commit adds time for such a connection attempt to time out. It also fixes some spurious logging of `this` that now refers to an object with an unhelpful `toString()` implementation introduced in #42636. Fixes #44073
6 tasks
colings86
removed
the
:Distributed Coordination/Cluster Coordination
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Jan 27, 2021
Today the discovery phase has a short 1-second timeout for handshaking with a remote node after connecting, which allows it to quickly move on and retry in the case of connecting to something that doesn't respond straight away (e.g. it isn't an Elasticsearch node). This short timeout was necessary when the component was first developed because each connection attempt would block a thread. Since elastic#42636 the connection attempt is now nonblocking so we can apply a more relaxed timeout. If transport security is enabled then our handshake timeout applies to the TLS handshake followed by the Elasticsearch handshake. If the TLS handshake alone takes over a second then the whole handshake times out with a `ConnectTransportException`, but this does not tell us which of the two individual handshakes took so long. TLS handshakes have their own 10-second timeout, which if reached yields a `SslHandshakeTimeoutException` that allows us to distinguish a problem at the TLS level from one at the Elasticsearch level. Therefore this commit extends the discovery probe timeouts.
DaveCTurner
added a commit
that referenced
this pull request
Jan 27, 2021
Today the discovery phase has a short 1-second timeout for handshaking with a remote node after connecting, which allows it to quickly move on and retry in the case of connecting to something that doesn't respond straight away (e.g. it isn't an Elasticsearch node). This short timeout was necessary when the component was first developed because each connection attempt would block a thread. Since #42636 the connection attempt is now nonblocking so we can apply a more relaxed timeout. If transport security is enabled then our handshake timeout applies to the TLS handshake followed by the Elasticsearch handshake. If the TLS handshake alone takes over a second then the whole handshake times out with a `ConnectTransportException`, but this does not tell us which of the two individual handshakes took so long. TLS handshakes have their own 10-second timeout, which if reached yields a `SslHandshakeTimeoutException` that allows us to distinguish a problem at the TLS level from one at the Elasticsearch level. Therefore this commit extends the discovery probe timeouts.
DaveCTurner
added a commit
that referenced
this pull request
Jan 27, 2021
Today the discovery phase has a short 1-second timeout for handshaking with a remote node after connecting, which allows it to quickly move on and retry in the case of connecting to something that doesn't respond straight away (e.g. it isn't an Elasticsearch node). This short timeout was necessary when the component was first developed because each connection attempt would block a thread. Since #42636 the connection attempt is now nonblocking so we can apply a more relaxed timeout. If transport security is enabled then our handshake timeout applies to the TLS handshake followed by the Elasticsearch handshake. If the TLS handshake alone takes over a second then the whole handshake times out with a `ConnectTransportException`, but this does not tell us which of the two individual handshakes took so long. TLS handshakes have their own 10-second timeout, which if reached yields a `SslHandshakeTimeoutException` that allows us to distinguish a problem at the TLS level from one at the Elasticsearch level. Therefore this commit extends the discovery probe timeouts.
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.
This PR converts the ConnectionManager's
openConnectionandconnectToNodemethods to async-style. This will allow us to not block threads anymore when opening connections. This PR also adapts the cluster coordination subsystem to make use of the new async APIs, allowing to remove some hacks in the test infrastructure that had to account for the previous synchronous nature of the connection APIs.