KAFKA-3373 add 'log' prefix to configurations in KIP-31/32#1049
KAFKA-3373 add 'log' prefix to configurations in KIP-31/32#1049becketqin wants to merge 2 commits into
Conversation
| val LogFlushOffsetCheckpointIntervalMsProp = "log.flush.offset.checkpoint.interval.ms" | ||
| val LogPreAllocateProp = "log.preallocate" | ||
| val MessageFormatVersionProp = "message.format.version" | ||
| val LogMessageFormatVersionProp = "log.message.format.version" |
There was a problem hiding this comment.
Do you think we can just reuse the definition in LogConfig and append "log."? This will make the relationship clearer and prevent accidental diversion.
sorry for nitpick, but after seeing all the places where same config has entirely different name in Log and Kafka, I'm a bit concerned.
There was a problem hiding this comment.
@gwenshap Good idea. That is probably less error prone. I will update the patch.
There was a problem hiding this comment.
@gwenshap After taking another look at the current code. It seems the naming is not as simple as a prefix + config name. Many configuration names are not changed in the LogConfig. For example,
log.roll.ms, log.roll.jitter.ms and log.index.size.max.bytes in the KafkaConfig become segment.ms, segment.jitter.ms and segment.index.bytes in the LogConfig.
Also, quite a few log.* configurations in KafkaConfig are not in LogConfig.
As of now I think we'd better just keep it as is but only make the new configurations using the log prefix + configuration name.
There was a problem hiding this comment.
Yeah, unfortunately we can't apply this now as a matter of rule.
I mean for the new configuration - can we use "log."+ LogConfig.MessageFormatVersionProp instead of "log.message.format.version".
Not a big deal, but seems like a simple improvement?
There was a problem hiding this comment.
@gwenshap Ah I see. Just updated the patch.
|
Thanks for the update. Changes look good. Just waiting for tests to finish running on my machine before I merge :) |
2 participants
No description provided.