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

Remove TcpChannel#setSoLinger method #35924

Merged
merged 1 commit into from
Nov 27, 2018

Conversation

Tim-Brooks
Copy link
Contributor

This commit removes the dedicated setSoLinger method. This simplifies
the TcpChannel interface. This method has very little effect as the
SO_LINGER is not set prior to the channels being closed in the abstract
transport test case. We still will set SO_LINGER on the
MockNioTransport. However we can do this manually.

This commit removes the dedicated `setSoLinger` method. This simplifies
the `TcpChannel` interface. This method has very little effect as the
SO_LINGER is not set prior to the channels being closed in the abstract
transport test case. We still will set SO_LINGER on the
`MockNioTransport`. However we can do this manually.
@Tim-Brooks Tim-Brooks added >non-issue :Distributed/Network Http and internode communication implementations v7.0.0 v6.6.0 labels Nov 26, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM

@s1monw
Copy link
Contributor

s1monw commented Nov 27, 2018

@tbrooks8 @original-brownbear I am not sure I follow. Didn't we add this to make some tests be more reliable? Now we only set this on the the MockNioTransport. Should we also set it manually on all the others?

@original-brownbear
Copy link
Member

@s1monw

turning off the linger has outlived its usefulness due to other changes. When I first added this a year ago it was applied to all channels before closing them which had a massive impact. This caused some trouble so the logic here https://github.com/elastic/elasticsearch/pull/35924/files#diff-3b8055db0941b6b23392b71384b69678L353 started checking for the life-cycle state so that we only RST from stopped TCP transports. This makes sense, but as @tbrooks8 points out is very rarely the case (we generally close first then stop).
=> this logic doesn't actually set linger a lot
Also, with @tbrooks8 lowering the connection count in tests a lot via some recent changes this isn't practically necessary any longer as far as I can see.

Should we also set it manually on all the others?

I don't think we should add this to the production ones. In practice their tests' (lingering) connection counts are low enough now to not cause trouble. We could maybe set linger to 0 for the blocking mock network implementation as well, but practically it doesn't seem necessary at this point. Also, there has been some recent discussion to maybe remove the blocking mock network altogether since its kind of redundant. See #34458 (comment)

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.

sounds good to me @original-brownbear thanks

@Tim-Brooks Tim-Brooks merged commit cc1fa79 into elastic:master Nov 27, 2018
Tim-Brooks added a commit to Tim-Brooks/elasticsearch that referenced this pull request Nov 27, 2018
This commit removes the dedicated `setSoLinger` method. This simplifies
the `TcpChannel` interface. This method has very little effect as the
SO_LINGER is not set prior to the channels being closed in the abstract
transport test case.
Tim-Brooks added a commit that referenced this pull request Nov 27, 2018
This commit removes the dedicated `setSoLinger` method. This simplifies
the `TcpChannel` interface. This method has very little effect as the
SO_LINGER is not set prior to the channels being closed in the abstract
transport test case.
@Tim-Brooks Tim-Brooks deleted the remove_so_linger branch December 18, 2019 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Network Http and internode communication implementations >non-issue v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants