KFKA-4340: follow up patch for KAFAK-4340#2544
KFKA-4340: follow up patch for KAFAK-4340#2544becketqin wants to merge 3 commits intoapache:trunkfrom
Conversation
becketqin
force-pushed
the
KAFKA-4340_follow_up
branch
from
03a3bee to
fe9c0f0
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
cc @ijuma |
becketqin
changed the title
…amp.difference.max.ms
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@becketqin : Thanks for the patch. LGTM. Just a couple of minor comments.
| <li>Several new fields including "security.protocol", "connections.max.idle.ms", "retry.backoff.ms", "reconnect.backoff.ms" and "request.timeout.ms" were added to | ||
| StreamsConfig class. User should pay attenntion to the default values and set these if needed. For more details please refer to <a href="#streamsconfigs">3.5 Kafka Streams Configs</a>.</li> | ||
| <li>The <code>offsets.topic.replication.factor</code> broker config is now enforced upon auto topic creation. Internal auto topic creation will fail with a GROUP_COORDINATOR_NOT_AVAILABLE error until the cluster size meets this replication factor requirement.</li> | ||
| <li>The default value of <code>message.timestamp.difference.max.ms</code> has been changed to the same as <code>retention.ms</code></li> |
There was a problem hiding this comment.
It seems that we need to add that to a new section for upgrade_1030_notable?
| recoveryPoint = 0L, | ||
| time.scheduler, | ||
| time) | ||
| log.append(MemoryRecords.withRecords(Record.create(-1L, "key".getBytes, "value".getBytes))) |
There was a problem hiding this comment.
Instead of -1, could we just reference Record.NO_TIMESTAMP?
|
Refer to this link for build results (access rights to CI server needed): |
|
Also, @ijuma brought up a good point. Right now, in KafkaConfig, if retention time is set, but MessageTimestampDifferenceMaxMs is not, MessageTimestampDifferenceMaxMs defaults to the default retention time. It seems that MessageTimestampDifferenceMaxMs should default to the customized retention time instead. |
|
Thanks for the patch, @becketqin. It looks good. @ijuma, @junrao, do you think we'll be able to fix this soon to get the tests passing again, or should we revert KAFKA-4340 so that we have more time to think about it and get it right? Right now trunk is broken. |
|
@cmccabe I'm working on the patch and will submit an update today. I am fine to revert the patch as it is not that critical to change the default configuration value. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Fixed the upgrade notes, tweaked the documentation of the config to mention the new default and merged to trunk. |
|
Refer to this link for build results (access rights to CI server needed): |
…fault log.retention.ms Author: Jiangjie Qin <becket.qin@gmail.com> Reviewers: Jun Rao <junrao@gmail.com>, Ismael Juma <ismael@juma.me.uk> Closes apache#2544 from becketqin/KAFKA-4340_follow_up
5 participants
No description provided.