-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
[Zen2] Add HandshakingTransportAddressConnector #32643
[Zen2] Add HandshakingTransportAddressConnector #32643
Conversation
The `PeerFinder`, introduced in elastic#32246, needs to be able to identify, and connect to, a remote master node using only its `TransportAddress`. This can be done by opening a single-channel connection to the address, performing a handshake, and only then forming a full-blown connection to the node. This change implements this logic.
Pinging @elastic/es-distributed |
@Override | ||
protected void doRun() throws Exception { | ||
|
||
// TODO if transportService is already connected to this address then skip the handshaking |
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.
@tbrooks8 @ywelsch I think you discussed this area recently. Arguably it might make sense for some (or more) of this functionality to move elsewhere. Perhaps within TransportService
itself as it currently stands although I know that you've plans for this area so that might change in future.
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.
A few nits, looks good otherwise.
IOUtils.closeWhileHandlingException(connection); | ||
} | ||
|
||
// NOMERGE better exceptions for failure cases? |
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.
maybe use ConnectTransportException
? We use that one e.g. when application-level handshake fails (see connectToNode
).
} | ||
|
||
void assertFailure() throws InterruptedException { | ||
assertTrue(completionLatch.await(1, TimeUnit.SECONDS)); |
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.
maybe this is a little optimistic for our slow CI machines. 30 secs?
Same comment for other places in this class.
.put(NODE_NAME_SETTING.getKey(), "node") | ||
.put(CLUSTER_NAME_SETTING.getKey(), "local-cluster") | ||
.build(); | ||
threadPool = new ThreadPool(settings); |
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.
use TestThreadPool instead?
@After | ||
public void stopServices() { | ||
transportService.stop(); | ||
threadPool.shutdown(); |
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.
use terminate(threadPool);
(this method is in ESTestCase)
Failure is #32215 not this PR. @elasticmachine retest this please. |
The
PeerFinder
, introduced in #32246, needs to be able to identify, andconnect to, a remote master node using only its
TransportAddress
. This can bedone by opening a single-channel connection to the address, performing a
handshake, and only then forming a full-blown connection to the node. This
change implements this logic.