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

Allow setting common TCP socket options #880

Merged
merged 2 commits into from Oct 2, 2017

Conversation

Projects
None yet
3 participants
@lukebakken
Contributor

lukebakken commented Sep 29, 2017

Includes and supercedes #864

It appears that the RabbitMQ Java client does not set TCP keepalives by default, but does disable Nagle's algorithm, so we should probably follow the lead there.

Also see #67 about using IPPROTO_TCP instead of 6

@codecov

This comment has been minimized.

codecov bot commented Sep 29, 2017

Codecov Report

Merging #880 into master will increase coverage by 0.1%.
The diff coverage is 86.36%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #880     +/-   ##
=========================================
+ Coverage   81.99%   82.09%   +0.1%     
=========================================
  Files          20       21      +1     
  Lines        3665     3719     +54     
  Branches      544      552      +8     
=========================================
+ Hits         3005     3053     +48     
- Misses        513      517      +4     
- Partials      147      149      +2
Impacted Files Coverage Δ
pika/connection.py 88.58% <100%> (+0.19%) ⬆️
pika/adapters/base_connection.py 64.98% <76.92%> (+0.27%) ⬆️
pika/tcp_socket_opts.py 84.37% <84.37%> (ø)
pika/compat.py 96.55% <87.5%> (-1.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73c72e3...8257a8e. Read the comment docs.

tcp_opts: Allow setting TCP keepalive options via pika parameters
tcp_opts: Add some tests for TCP keepalive options

tcp_opts: Fix compatibility issues on tcp socket options

tcp_opts: Mock TCP_KEEPIDLE in tests

tcp_opts: Documentation improvements

Move TCP options code to own module

@lukebakken lukebakken force-pushed the pika-tcp-options-864 branch from 693104d to 979278a Oct 1, 2017

@lukebakken lukebakken requested a review from michaelklishin Oct 1, 2017

Change default TCP_USER_TIMEOUT to 60
The option has basically the same meaning as heartbeat timeout
setting in AMQP 0-9-1, STOMP, etc [1][2]. Default heartbeat timeout for
RabbitMQ is currently 60 seconds.

1. http://man7.org/linux/man-pages/man7/tcp.7.html
2. http://www.rabbitmq.com/heartbeats.html

@michaelklishin michaelklishin merged commit 5ceaef1 into master Oct 2, 2017

6 checks passed

codecov/patch 86.36% of diff hit (target 81.99%)
Details
codecov/project 82.09% (+0.1%) compared to 73c72e3
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@lukebakken lukebakken deleted the pika-tcp-options-864 branch Oct 2, 2017

@lukebakken lukebakken added this to the 0.11.1 milestone Oct 2, 2017

@katajakasa

This comment has been minimized.

Contributor

katajakasa commented on pika/tcp_socket_opts.py in 8257a8e Oct 4, 2017

Note that the value you set here is not default value for the option. It is the tcp option identifier (see https://github.com/torvalds/linux/blob/master/include/uapi/linux/tcp.h). This change breaks TCP_USER_TIMEOUT if python library does not have TCP_USER_TIMEOUT set and the OS is linux.

This comment has been minimized.

Contributor

lukebakken replied Oct 4, 2017

Thank you for noticing this. I will revert it.

gbartl pushed a commit to gbartl/pika that referenced this pull request Jan 27, 2018

Merge pull request pika#880 from pika/pika-tcp-options-864
Allow setting common TCP socket options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment