Skip to content

KAFKA-724; Allow automatic socket.send.buffer from operating system in SocketServer#1469

Closed
rekhajoshm wants to merge 7 commits intoapache:trunkfrom
rekhajoshm:KAFKA-724-rebased
Closed

KAFKA-724; Allow automatic socket.send.buffer from operating system in SocketServer#1469
rekhajoshm wants to merge 7 commits intoapache:trunkfrom
rekhajoshm:KAFKA-724-rebased

Conversation

@rekhajoshm
Copy link
Copy Markdown
Contributor

@rekhajoshm rekhajoshm commented Jun 5, 2016

If socket.receive.buffer.bytes/socket.send.buffer.bytes are set to -1, use the OS defaults.

@rekhajoshm
Copy link
Copy Markdown
Contributor Author

@ijuma PR #50 was done a while back, changes after rebase.Please have a look. thanks

if(readBufferSize != Selectable.USE_DEFAULT_BUFFER_SIZE)
channel.socket.setReceiveBufferSize(readBufferSize)
if(writeBufferSize > 0)
if(writeBufferSize != Selectable.USE_DEFAULT_BUFFER_SIZE)
Copy link
Copy Markdown
Member

@ijuma ijuma Jun 5, 2016

Choose a reason for hiding this comment

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

This is a change in behaviour (-2 would throw an exception instead of using the OS default) and BlockingChannel is deprecated, so I would prefer not to change it.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 5, 2016

Thanks for the PR @rekhajoshm. I left a couple of minor comments. Also, the PR title and description are used in the merged commit message. Can you please edit them to be:

PR title: KAFKA-724: Allow automatic socket.send.buffer from operating system in SocketServer

PR description: If socket.receive.buffer.bytes/socket.send.buffer.bytes are set to -1, use the OS defaults.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 5, 2016

One more thing: we should also update the documentation in KafkaConfig and CommonClientConfigs stating that if the value is -1, the OS default is used.

@rekhajoshm rekhajoshm changed the title KAFKA-724; Send, receive buffer size set if not -1 KAFKA-724; Allow automatic socket.send.buffer from operating system in SocketServer Jun 6, 2016
@rekhajoshm
Copy link
Copy Markdown
Contributor Author

done @ijuma Please have a look. thanks.

import kafka.utils._
import org.apache.kafka.common.metrics._
import org.apache.kafka.common.network.{ChannelBuilders, KafkaChannel, LoginType, Mode, Selector => KSelector}
import org.apache.kafka.common.network.{Selector => KSelector, _}
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.

We prefer to avoid wildcard imports.

@rekhajoshm
Copy link
Copy Markdown
Contributor Author

Thanks @ijuma Please have a look. thanks

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 6, 2016

The PR build failed @rekhajoshm.

@rekhajoshm
Copy link
Copy Markdown
Contributor Author

my bad.fixed @ijuma Please have a look. thanks

@asfgit asfgit closed this in 430bf56 Jun 7, 2016
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 7, 2016

Thanks, LGTM. Merged to trunk.

@slaunay
Copy link
Copy Markdown
Contributor

slaunay commented Jun 8, 2016

I am interested in such feature for configuring a producer but using -1 throws the following exception with Kafka clients 0.9.0.1:

Exception in thread "main" org.apache.kafka.common.config.ConfigException: Invalid value -1 for configuration receive.buffer.bytes: Value must be at least 0
    at org.apache.kafka.common.config.ConfigDef$Range.ensureValid(ConfigDef.java:308)
    at org.apache.kafka.common.config.ConfigDef.parse(ConfigDef.java:153)
    at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:49)
    at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:56)
    at org.apache.kafka.clients.producer.ProducerConfig.<init>(ProducerConfig.java:315)
    at org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:156)
    ...

I believe this is caused by an atLeast(0) validation in ProducerConfig.java that is not present in KafkaConfig for the server configuration:

  .define(SEND_BUFFER_CONFIG, Type.INT, 128 * 1024, atLeast(0), Importance.MEDIUM, CommonClientConfigs.SEND_BUFFER_DOC)
  .define(RECEIVE_BUFFER_CONFIG, Type.INT, 32 * 1024, atLeast(0), Importance.MEDIUM, CommonClientConfigs.RECEIVE_BUFFER_DOC)

I can create a separate PR for this but because CommonClientConfigs.java documentation was modified I am not sure if this feature was targeted at the server only or both the server and the clients.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 8, 2016

@slaunay Yes, please create a PR.

asfgit pushed a commit that referenced this pull request Jun 16, 2016
Follow up on KAFKA-724 (#1469) to allow OS socket buffer sizes auto tuning for both the broker and the clients.

Author: Sebastien Launay <sebastien@opendns.com>

Reviewers: Sriharsha Chintalapani <harsha@hortonworks.com>, Grant Henke <granthenke@gmail.com>, Ismael Juma <ismael@juma.me.uk>

Closes #1507 from slaunay/enhancement/os-socket-buffer-size-tuning-for-clients
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.

3 participants