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

Add configuration support to override socket options. #537

Merged
merged 4 commits into from May 26, 2015
Merged

Add configuration support to override socket options. #537

merged 4 commits into from May 26, 2015

Conversation

btatnall
Copy link
Contributor

I need to change the send and receive buffer sizes.

@btatnall
Copy link
Contributor Author

The Failed Build Job seems to be completely unrelated to the changes that I've made. I'm not able to reproduce consistently.

@petergoldstein
Copy link
Owner

@btatnall Why do you need to change the send and receive buffer sizes?

I'm fairly reluctant to expose socketopts in this way, as this has not been an expressed need in the user community, and adds substantial opportunity for user error. I'd need to hear a compelling use case or two before adding this complexity - even optional complexity - to Dalli master.

@btatnall
Copy link
Contributor Author

@petergoldstein I'm on a 10g network and the default buffer sizes are far too small for the size and rate of data that I'm pushing. Because I can't change the buffer sizes I'm seeing frequent timeouts. I do not want to change the system's defaults in rmem_default and wmem_default as that will effect many other applications running on the same machine. Changing buffers on the socket which is experiencing the pressure doesn't effect anything else.

I understand exposing Socket::Option may be too much. Would you consider exposing SO_RCVBUF and SO_SNDBUF in the same way that keepalive is exposed? Projects like unicorn expose these options as well.

INFO memcached-node-10:11211 failed (count: 0) Timeout::Error: IO timeout: {:host=>"memcached-node-10", :port=>11211, :down_retry_delay=>1, :socket_timeout=>0.5, :socket_max_failures=>2, :socket_failure_delay=>0.01, :value_max_bytes=>41943040, :compressor=>Dalli::Compressor, :compression_min_size=>1024, :compression_max_size=>false, :serializer=>Marshal, :username=>nil, :password=>nil, :keepalive=>true, :pool_size=>30, :compress=>nil, :threadsafe=>false}

@petergoldstein
Copy link
Owner

@btatnall I'd find that sort of approach a lot more acceptable. Can you redo the PR with that in mind?

@mperham
Copy link
Collaborator

mperham commented May 22, 2015

Given that memcached typically runs on gigabit networks these days, perhaps we should just up the default socket buffer size rather than letting people tune it. What's the default and what do you want to set it to?

@btatnall
Copy link
Contributor Author

@petergoldstein I'll redo the pull request. Exposing SNDBUF and RCVBUF explicitly.

@mperham TBH I'd let the linux distribution determine the default buffer sizes. This isn't a scenario where forcing every client to use the same settings is beneficial. For example, I have a few machines that default to 65Kb and others that default to 262Kb and another that defaults to 12.5Mb and another at 65Mb, which is why using net.core.rmem_default and net.core.wmem_default works for most scenarios

- only expose sndbuf and rcvbuf socket options
@btatnall
Copy link
Contributor Author

@petergoldstein Changes committed. Travis failures seem completely unrelated (seems a few of these tests fail regularly on Travis).

petergoldstein added a commit that referenced this pull request May 26, 2015
Add configuration support to override socket options.
@petergoldstein petergoldstein merged commit 819dc9d into petergoldstein:master May 26, 2015
@petergoldstein
Copy link
Owner

Looks good. Thanks!

@btatnall
Copy link
Contributor Author

@petergoldstein If you don't mind me asking. What are the conditions to get a new release pushed?

@petergoldstein
Copy link
Owner

@btatnall There are no formal conditions per se - it's up to the judgment of the maintainer and their availability. I'm not averse to doing a release with this soon (although I'll probably want to review the issue/PR list before finalizing the release), but I'm pretty busy this week and next. It'd probably have to wait for the 2nd week in June.

@btatnall
Copy link
Contributor Author

@petergoldstein Is it possible to create a release with just this change in it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants