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
KAFKA-13270: Set JUTE_MAXBUFFER to 4 MB by default #11295
Conversation
@omkreddy if you have cycles, this is a 3.0 blocker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ijuma Thanks for the PR. I left a few nits.
getZooKeeperClientProperty(zkClientConfig, ZkClientCnxnSocketProp).isDefined && | ||
getZooKeeperClientProperty(zkClientConfig, ZkSslKeyStoreLocationProp).isDefined | ||
private[kafka] def zkTlsClientAuthEnabled(zkClientConfig: ZKClientConfig): Boolean = { | ||
zooKeeperClientProperty(zkClientConfig, ZkSslClientEnableProp).map(_ == "true").getOrElse(false) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We could use exists
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, even better: contains
. :)
val prefixedValue = configMap.get(AclAuthorizer.configPrefix + kafkaProp) | ||
if (prefixedValue.isDefined) | ||
zkClientConfig.get.setProperty(sysProp, | ||
KafkaConfig.ZkSslConfigToSystemPropertyMap.forKeyValue { (kafkaProp, sysProp) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We could remove the extra block defined by the last {
on this line.
val client1 = KafkaZkClient(zkConnect, zkAclsEnabled.getOrElse(JaasUtils.isZkSaslEnabled), zkSessionTimeout, | ||
zkConnectionTimeout, zkMaxInFlightRequests, Time.SYSTEM, name = "KafkaZkClient", | ||
zkClientConfig = clientConfig1) | ||
try assertEquals("2048000", client1.currentZooKeeper.getClientConfig.getProperty(ZKConfig.JUTE_MAXBUFFER)) | ||
finally client1.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth defining an inner helper method to avoid the repeated code here? Something like: assertPropery(config: ZKClientConfig, property: String, expectedValue: String)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair suggestion, I pushed a change along these lines.
@dajac Thanks for the review, I pushed a commit that addresses your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
We restore the 3.4.x/3.5.x behavior unless the caller has set the property (note that ZKConfig auto configures itself if certain system properties have been set). I added a unit test that fails without the change and passes with it. I also refactored the code to streamline the way we handle parameters passed to KafkaZkClient and ZooKeeperClient. See apache/zookeeper#1129 for the details on why the behavior changed in 3.6.0. Credit to @rondagostino for finding and reporting this issue. Reviewers: David Jacot <djacot@confluent.io>
We restore the 3.4.x/3.5.x behavior unless the caller has set the property (note that ZKConfig auto configures itself if certain system properties have been set). I added a unit test that fails without the change and passes with it. I also refactored the code to streamline the way we handle parameters passed to KafkaZkClient and ZooKeeperClient. See apache/zookeeper#1129 for the details on why the behavior changed in 3.6.0. Credit to @rondagostino for finding and reporting this issue. Reviewers: David Jacot <djacot@confluent.io>
ZooKeeper 3.6.0 changed the default configuration for JUTE_MAXBUFFER from 4 MB to 1 MB.
This causes a regression if Kafka tries to retrieve a large amount of data across many
znodes – in such a case the ZooKeeper client will repeatedly emit a message of the form
"java.io.IOException: Packet len <####> is out of range".
We restore the 3.4.x/3.5.x behavior unless the caller has set the property (note that ZKConfig
auto configures itself if certain system properties have been set).
I added a unit test that fails without the change and passes with it.
I also refactored the code to streamline the way we handle parameters passed to
KafkaZkClient and ZooKeeperClient.
See apache/zookeeper#1129 for the details on why the behavior
changed in 3.6.0.
Credit to @rondagostino for finding and reporting this issue.
Committer Checklist (excluded from commit message)