Skip to content

KAFKA-3840; Allow clients default OS buffer sizes#1507

Closed
slaunay wants to merge 3 commits intoapache:trunkfrom
slaunay:enhancement/os-socket-buffer-size-tuning-for-clients
Closed

KAFKA-3840; Allow clients default OS buffer sizes#1507
slaunay wants to merge 3 commits intoapache:trunkfrom
slaunay:enhancement/os-socket-buffer-size-tuning-for-clients

Conversation

@slaunay
Copy link
Copy Markdown
Contributor

@slaunay slaunay commented Jun 14, 2016

Follow up on KAFKA-724 (#1469) to allow OS socket buffer sizes auto tuning for both the broker and the clients.

@harshach
Copy link
Copy Markdown

+1

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 15, 2016

Thanks for the PR. It would be good to add a unit test for this.

@granthenke
Copy link
Copy Markdown
Member

At some point should we consider changing the default to -1 as well?

@slaunay
Copy link
Copy Markdown
Contributor Author

slaunay commented Jun 15, 2016

👍
I agree that -1 is a more sensible default especially when running an untuned producer on Linux over a long fat network instead of the default 128 KiB for send.buffer.bytes.

I believe the default net.core.wmem_max is often set to 256 KiB preventing the SO_SNDBUF to be really set to something higher than 256 KiB (technically it will be bound to 512 KiB as the Linux kernel double the size internally).
On the other hand, from what I have seen if you do not set SO_SNDBUF (with send.buffer.bytes set to -1) then you are not bound by net.core.wmem_max anymore but by net.ipv[46].tcp_wmem which is often set to a 4 MiB max size that tremendously increase throughput on high latency network (obviously after configuring lingering and batching accordingly).

My current workaround consists of setting net.core.wmem_max to match the max of net.ipv[46].tcp_wmem and only then I can really use send.buffer.bytes with something bigger than 256 KiB.

I found that using -1 allows to get better performance out of the box on Linux while allowing system engineers to do some TCP tuning with or without configuring the application.

The same applies to SO_RCVBUF although it is probably less common to run a consumer over a long fat network.

config.put(ConsumerConfig.SEND_BUFFER_CONFIG, Selectable.USE_DEFAULT_BUFFER_SIZE);
config.put(ConsumerConfig.RECEIVE_BUFFER_CONFIG, Selectable.USE_DEFAULT_BUFFER_SIZE);
config.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, ByteArrayDeserializer.class);
config.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, ByteArrayDeserializer.class);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nitpick: if you pass the deserializers via the constructor, it's a bit more concise. This applies to all tests.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 15, 2016

Thanks for adding the tests. I left a few minor comments that should hopefully be easy to address. With regards to changing the default, it's worth starting a mailing list thread to get input from a wider group.

- use concise constructor
- remove exception message validation
- move try/catch/fail into @test(expected = ...)
@slaunay
Copy link
Copy Markdown
Contributor Author

slaunay commented Jun 15, 2016

I did base the unit tests code on existing style from the neighbor tests but I changed it to @Test(expected = ...) that's more concise if we do not care about the exception message validation.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 15, 2016

Thanks, LGTM.

@asfgit asfgit closed this in 54ba228 Jun 16, 2016
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 16, 2016

Tests passed locally, merged to trunk.

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.

4 participants