Skip to content
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

Detach handshake from connect to node #22037

Merged
merged 6 commits into from Dec 10, 2016

Conversation

Projects
None yet
2 participants
@s1monw
Copy link
Contributor

commented Dec 7, 2016

Today we connect and publish the nodes connection before we execute a
handshake with the node we connect to. In the case of connecting to a node
that won't pass the handshake this connection is already published and other
code paths can use it. This commit detaches the connection and the publish of the
connection such that TransportService can do a handshake before actually connect
and publish the connection.

Detach handshake from connect to node
Today we connect and publish the nodes connection before we execute a
handshake with the node we connect to. In the case of connecting to a node
that won't pass the handshake this connection is already `published` and other
code paths can use it. This commit detaches the connection and the publish of the
connection such that `TransportService` can do a handshake before actually connect
and publish the connection.
@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2016

@elasticmachine test this please

@jasontedor jasontedor self-assigned this Dec 7, 2016

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2016

@elasticmachine test this please

@jasontedor
Copy link
Member

left a comment

I like the change @s1monw. I left some comments.

@@ -330,36 +330,35 @@ public DiscoveryNode connectToNodeAndHandshake(
public DiscoveryNode connectToNodeAndHandshake(
final DiscoveryNode node,
final long handshakeTimeout,
final boolean checkClusterName) {
final boolean checkClusterName) throws IOException {

This comment has been minimized.

Copy link
@jasontedor

jasontedor Dec 9, 2016

Member

This parameter was initially added to support handshaking in the transport client but that was later removed in a follow-up and this parameter was left behind. I think that it can be dropped now. We can do it here, or a follow-up, it doesn't matter to me, but since you already edited the call-sites I think the IDE should make just dropping this parameter easy here.

This comment has been minimized.

Copy link
@s1monw

s1monw Dec 9, 2016

Author Contributor

I can remove it here but I think we will likely add it back in the future when we try to connect to foreign clusters. Lets trash it it's as simple to add back as it is to remove

transport.disconnectFromNode(node);
throw e;
DiscoveryNode handshakeNode;
try (Transport.Connection connection = transport.openConnection(node, ConnectionProfile.LIGHT_PROFILE)){

This comment has been minimized.

Copy link
@jasontedor

jasontedor Dec 9, 2016

Member

Nit: )){ -> )) { (whitespace)


/**
* Opens a new connection to the given node and returns it. In contrast to {@link #connectToNode(DiscoveryNode, ConnectionProfile)}
* is the returned connection not managed by the transport implementation. This connection must be closed once it's not needed anymore.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Dec 9, 2016

Member

Nit: is the returned connection not managed -> the returned connection is not managed

DiscoveryNode getNode();

/**
* Sends the request to the node this connection is associtated with

This comment has been minimized.

Copy link
@jasontedor

jasontedor Dec 9, 2016

Member

Nit: associtated -> associated


void onNodeDisconnected(DiscoveryNode node);
/**
* Called once a node connection closed and unregistered

This comment has been minimized.

Copy link
@jasontedor

jasontedor Dec 9, 2016

Member

Nit: closed -> is closed, add period.

@@ -364,7 +353,24 @@ public Channel channel(TransportRequestOptions.Type type) {

@Override
public synchronized void close() throws IOException {
closeChannels(Arrays.asList(channels).stream().filter(Objects::nonNull).collect(Collectors.toList()));
if (closed.compareAndSet(false, true)) {
closeChannels(Arrays.asList(channels).stream().filter(Objects::nonNull).collect(Collectors.toList()));

This comment has been minimized.

Copy link
@jasontedor

jasontedor Dec 9, 2016

Member

Arrays.asList(channels).stream() -> Arrays.stream(channels)

Transport.Connection connection = getConnection(node);
sendRequest(connection, action, request, options, futureHandler);
} catch (NodeNotConnectedException ex) {
// handle the NNCException from the getConnection - the caller might not handle it so we invoke the handler

This comment has been minimized.

Copy link
@jasontedor

jasontedor Dec 9, 2016

Member

Maybe just shorten this comment to the caller might not handle this so we invoke the handler (and elsewhere)?

s1monw added some commits Dec 9, 2016

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2016

@jasontedor I pushed a new commit

@jasontedor
Copy link
Member

left a comment

LGTM.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2016

@elasticmachine test this please

@s1monw s1monw merged commit 01d67e0 into elastic:master Dec 10, 2016

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@s1monw s1monw deleted the s1monw:detach_connection_from_publish branch Dec 10, 2016

s1monw added a commit that referenced this pull request Dec 10, 2016

Detach handshake from connect to node (#22037)
Today we connect and publish the nodes connection before we execute a
handshake with the node we connect to. In the case of connecting to a node
that won't pass the handshake this connection is already `published` and other
code paths can use it. This commit detaches the connection and the publish of the
connection such that `TransportService` can do a handshake before actually connect
and publish the connection.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.