MINOR: Raft max batch size needs to propagate to log config#10256
Conversation
|
Do we need a test? |
|
@ijuma Yes, probably. I'm working on it. |
mumrah
left a comment
mumrah
left a comment
There was a problem hiding this comment.
Thanks @hachikuji! A few questions inline
| LogConfig.validateValues(props) | ||
| val defaultLogConfig = LogConfig(props) | ||
|
|
||
| val log = Log( |
There was a problem hiding this comment.
👍 Makes sense to move this here since KafkaMetadataLog fully owns the Log's lifecycle
| } | ||
|
|
||
| private def buildMetadataLog(): KafkaMetadataLog = { | ||
| val defaultProps = LogConfig.extractLogConfigMap(config) |
There was a problem hiding this comment.
I see we are no longer calling this, but rather explicitly populating the log properties. Are there other log configs we might want to expose?
There was a problem hiding this comment.
It seemed best for now to not allow overrides since this is a new usage of Log and we haven't had time to understand the impact of all of the configurations for this usage. Most of them are probably safe, but others do not even make sense (e.g. the retention settings). Let me open a JIRA so that we can consider which configs we want to expose for the metadata log and how we want to expose them.
There was a problem hiding this comment.
I filed this JIRA: https://issues.apache.org/jira/browse/KAFKA-12423.
This patch ensures that the constant max batch size defined in `KafkaRaftClient` is propagated to the constructed log configuration in `KafkaMetadataLog`. We also ensure that the fetch max size is set consistently with appropriate testing. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, David Arthur <mumrah@gmail.com>
4 participants
We increased the max batch size to 8MB in #10063, but we forgot to propagate the change to the
Loginstance which does its own validation. I had actually noted it in this comment #10063 (review), but I forgot about it afterwards.Committer Checklist (excluded from commit message)