Skip to content

Conversation

@Tim-Brooks
Copy link
Contributor

This commit is a follow up to the work completed in #27132. Essentially
it transitions two more methods (sendMessage and getLocalAddress) from
TcpTransport to TcpChannel. With this change, there is no longer a need for
TcpTransport to be aware of the specific type of channel a transport
returns. So that class is no longer parameterized by channel type.

This commit is a follow up to the work completed in elastic#27132. Essentially
it transitions two more methods (sendMessage and getLocalAddress) from
Transport to TcpChannel. With this change, there is no longer a need for
TcpTransport to be aware of the specific type of channel a transport
returns. So that class is no longer parameterized by channel type.
@Tim-Brooks
Copy link
Contributor Author

As a note, I have realized that it does not make sense for NioChannel to extend TcpChannel. NioChannel is selectable channel that can be handled by nio. TcpChannel is a channel that provides the functionality for our tcp binary protocol. We eventually will have some http channel that is an NioChannel, but not a TcpChannel. Right now this causes some issues for parameterized listener types in the nio transport. Once this is merged, I will follow this PR up with another PR that fixes this issue.

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so much goodness! LGTM

}

@Override
public void sendMessage(BytesReference reference, ActionListener<TcpChannel> listener) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a more verbose TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I'm not sure what happened to the rest of the comment.


@Override
public void sendMessage(BytesReference reference, ActionListener<TcpChannel> listener) {
// TODO: temporar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't you just use the passed in listener?

Copy link
Contributor Author

@Tim-Brooks Tim-Brooks Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. ActionListener<TcpChannel> != ActionListener<NioChannel> to the compiler. I will fix this in a follow up (#27407 (comment)).

@Tim-Brooks Tim-Brooks merged commit 80ef9bb into elastic:master Nov 16, 2017
Tim-Brooks added a commit that referenced this pull request Nov 16, 2017
This commit is a follow up to the work completed in #27132. Essentially
it transitions two more methods (sendMessage and getLocalAddress) from
Transport to TcpChannel. With this change, there is no longer a need for
TcpTransport to be aware of the specific type of channel a transport
returns. So that class is no longer parameterized by channel type.
Tim-Brooks added a commit to Tim-Brooks/elasticsearch that referenced this pull request Nov 17, 2017
This is a followup to elastic#27407. That commit removed the channel type
parameter from TcpTransport. This commit removes the parameter from the
handshake response handler.
Tim-Brooks added a commit that referenced this pull request Nov 17, 2017
This is a followup to #27407. That commit removed the channel type
parameter from TcpTransport. This commit removes the parameter from the
handshake response handler.
Tim-Brooks added a commit that referenced this pull request Nov 17, 2017
This is a followup to #27407. That commit removed the channel type
parameter from TcpTransport. This commit removes the parameter from the
handshake response handler.
@Tim-Brooks Tim-Brooks deleted the remove_parameter_from_tcp_transport branch December 10, 2018 16:19
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed Coordination/Network Http and internode communication implementations >non-issue v6.1.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants