KAFKA-18068: Fixing typo in ProducerConfig #17908
Conversation
|
@robvadai thanks for this patch. those configs are public so we need KIP before fixing them |
| public static final String PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_CONFIG = "partitioner.adaptive.partitioning.enable"; | ||
| private static final String PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_DOC = | ||
| "When set to 'true', the producer will try to adapt to broker performance and produce more messages to partitions hosted on faster brokers. " | ||
| public static final String PARTITIONER_ADAPTIVE_PARTITIONING_ENABLE_CONFIG = "partitioner.adaptive.partitioning.enable"; |
There was a problem hiding this comment.
Strictly speaking, this typo has formed part of the public interface since it was introduced in 2022. So, really, you should leave the existing constants in place and mark them as deprecated. They can then be removed in AK 5.0. Otherwise, any applications which happened to use the wrong spelling would fail to build.
| + "If 'false', producer will try to distribute messages uniformly. Note: this setting has no effect if a custom partitioner is used"; | ||
|
|
||
| /** <code>partitioner.availability.timeout.ms</code> */ | ||
| public static final String PARTITIONER_AVAILABILITY_TIMEOUT_MS_CONFIG = "partitioner.availability.timeout.ms"; | ||
| private static final String PARTITIONER_AVAILABILITY_TIMEOUT_MS_DOC = | ||
| "If a broker cannot process produce requests from a partition for <code>" + PARTITIONER_AVAILABILITY_TIMEOUT_MS_CONFIG + "</code> time, " | ||
| + "the partitioner treats that partition as not available. If the value is 0, this logic is disabled. " | ||
| + "Note: this setting has no effect if a custom partitioner is used or <code>" + PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_CONFIG | ||
| + "Note: this setting has no effect if a custom partitioner is used or <code>" + PARTITIONER_ADAPTIVE_PARTITIONING_ENABLE_CONFIG | ||
| + "</code> is set to 'false'"; |
There was a problem hiding this comment.
nit: Please end the doc string with ".".
| "When set to 'true', the producer will try to adapt to broker performance and produce more messages to partitions hosted on faster brokers. " | ||
| public static final String PARTITIONER_ADAPTIVE_PARTITIONING_ENABLE_CONFIG = "partitioner.adaptive.partitioning.enable"; | ||
| private static final String PARTITIONER_ADAPTIVE_PARTITIONING_ENABLE_DOC = | ||
| "When set to 'true', the producer will try to adapt to broker performance and produce more messages to partitions hosted on faster brokers." | ||
| + "If 'false', producer will try to distribute messages uniformly. Note: this setting has no effect if a custom partitioner is used"; |
There was a problem hiding this comment.
nit: Please end the doc string with ".".
| @@ -380,7 +380,7 @@ public class ProducerConfig extends AbstractConfig { | |||
| .define(COMPRESSION_LZ4_LEVEL_CONFIG, Type.INT, CompressionType.LZ4.defaultLevel(), CompressionType.LZ4.levelValidator(), Importance.MEDIUM, COMPRESSION_LZ4_LEVEL_DOC) | |||
| .define(COMPRESSION_ZSTD_LEVEL_CONFIG, Type.INT, CompressionType.ZSTD.defaultLevel(), CompressionType.ZSTD.levelValidator(), Importance.MEDIUM, COMPRESSION_ZSTD_LEVEL_DOC) | |||
| .define(BATCH_SIZE_CONFIG, Type.INT, 16384, atLeast(0), Importance.MEDIUM, BATCH_SIZE_DOC) | |||
| .define(PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_CONFIG, Type.BOOLEAN, true, Importance.LOW, PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_DOC) | |||
| .define(PARTITIONER_ADAPTIVE_PARTITIONING_ENABLE_CONFIG, Type.BOOLEAN, true, Importance.LOW, PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_DOC) | |||
There was a problem hiding this comment.
If I'm reading this correctly, you need to change both occurrences of ADPATIVE to ADAPTIVE and you've only done the first.
|
@chia7712 is correct. It would need a KIP too, which would be a trivial KIP so not much effort. |
|
Actually, thinking more about this, KIP-794 https://cwiki.apache.org/confluence/plugins/servlet/mobile?contentId=191336857#content/view/191336857 defines the config without the spelling mistake. This means this PR can define the correct spelling without a new KIP. A KIP will be needed to deprecate and then remove the erroneous constant but we can do that afterwards. |
The incorrect spelling is released in 3.3.0, so it would be nice to have a KIP to start the deprecation cycle in order to not break users' application in 4.x. @robvadai Do you have free cycle to file a KIP? |
|
@chia7712 will do eventually, just need to get familiar with the process, I'll use this guide: https://cwiki.apache.org/confluence/display/kafka/kafka+improvement+proposals |
|
@chia7712 I meant that we could temporarily have both spellings starting in AK 4.0. Something like: or That wouldn't break the user's code at all. And as soon as we have a KIP, we can begin the deprecation cycle on the old name. |
sorry that I didn't get your point :(
If the KIP isn't completed in time (@robvadai please prove me wrong), we'll end up with two configs (PARTITIONER_ADPATIVE_PARTITIONING_ENABLE_CONFIG and PARTITIONER_ADAPTIVE_PARTITIONING_ENABLE_CONFIG) in 4.0. Since this isn't a critical issue, perhaps we can honor the KIP process? |
Fixed typo in ProducerConfig.
The variables and descriptions were not tested so the fact it can be compiled tests integrity.
Committer Checklist (excluded from commit message)