Hello Jun,
Thanks for the reply.
JR42, JR45, JR46, JR47: I have updated the KIP.
JR43: If bootstrap.servers is empty for these configurations,
it will fail when creating the AdminClient. I have updated the KIP to reflect
this behavior.
JR44: Thanks for pointing this out. Kafka will indeed fail at the storage
format phase
if the configuration is invalid. I have updated the KIP accordingly.
JR48: The current in() method allows empty lists by default, so I believe
setting isEmptyAllowed to true
better preserves the original behavior. Therefore, I prefer not to change this
default.
Best Regards,
Jiunn-Yang
> Jun Rao <j...@confluent.io.INVALID> 於 2025年8月2日 凌晨2:38 寫道：
>
> Hi, Jiunn-Yang,
>
> Thanks for the reply. A few more comments.
>
> JR42. [6] [7] are not needed since currently an empty list already causes
> an exception.
>
> JR43. Could you document the current behavior when bootstrap.servers is
> empty in MirrorClientConfig and WorkerConfig?
>
> JR44. [12] seems incorrect. I set log.dir, but left log.dirs empty and got
> the following.
> bin/kafka-storage.sh format --standalone -t $KAFKA_CLUSTER_ID -c
> config/server.properties
> Exception in thread "main" java.lang.IllegalArgumentException: requirement
> failed: At least one log directory must be defined via log.dirs or log.dir.
>
> bin/kafka-server-start.sh config/server.properties
> [2025-08-01 11:31:20,097] INFO Registered
> `kafka:type=kafka.Log4jController` MBean
> (kafka.utils.Log4jControllerRegistration$)
> [2025-08-01 11:31:20,238] ERROR Exiting Kafka due to fatal exception
> (kafka.Kafka$)
> java.lang.IllegalArgumentException: requirement failed: At least one log
> directory must be defined via log.dirs or log.dir.
>
> JR45. Could you group all configs for WorkerConfig together?
>
> JR46. plugin.path: We have the following code. So, currently, it seems that
> an empty list is allowed for plugin.path and we treat it by adding the
> parent path of the classloader. It would be useful to justify why
> disallowing empty is better for the users.
>
> public static Set<PluginSource> pluginSources(Set<Path>
> pluginLocations, ClassLoader classLoader, PluginClassLoaderFactory factory)
> {
> Set<PluginSource> pluginSources = new LinkedHashSet<>();
> for (Path pluginLocation : pluginLocations) {
> try {
> pluginSources.add(isolatedPluginSource(pluginLocation,
> classLoader, factory));
> } catch (InvalidPathException | MalformedURLException e) {
> log.error("Invalid path in plugin path: {}. Ignoring.",
> pluginLocation, e);
> } catch (IOException e) {
> log.error("Could not get listing for plugin path: {}.
> Ignoring.", pluginLocation, e);
> }
> }
> pluginSources.add(classpathPluginSource(classLoader.getParent()));
> return pluginSources;
> }
>
> JR47. typo in "Should not allow setting validStrings is empty"
>
> JR48. We should pass in isEmptyAllowed as false in the code below, right?
> public static ValidList in(String... validStrings) {
> if (validStrings.length == 0) {
> throw new IllegalArgumentException("Valid strings list cannot
> be empty for in validator");
> }
> return new ValidList(List.of(validStrings), true, false);
> }
>
> Jun
>
> On Tue, Jul 29, 2025 at 8:53 AM 黃竣陽 <s7133...@gmail.com> wrote:
>
>> Hello Jun,
>>
>> Sorry for the late reply,
>>
>> JR40, JR41: I have updated the KIP based on your comments. Please take a
>> look.
>>
>> Best Regards,
>> Jiunn-Yang
>>
>>> Jun Rao <j...@confluent.io.INVALID> 於 2025年7月24日 凌晨2:56 寫道：
>>>
>>> Hi, Jiunn-Yang,
>>>
>>> Thanks for the reply.
>>>
>>> JR40. In the table, there are a few configs for which we now disallow
>> empty
>>> lists. It would be useful to document the current behavior for each of
>> them
>>> more accurately. It seems that they should all lead to some bad behavior.
>>> For example, if log.dirs is empty, it seems that we can't create topics?
>>> If group.coordinator.rebalance.protocols is empty, the group rebalance
>>> protocol will fail?
>>>
>>> JR41. It would be useful to clean up the "Compatibility, Deprecation, and
>>> Migration Plan" section a bit.
>>> JR41.1 The migration plan has "Introduce a new properties isEmptyAllowed
>> ,
>>> isNullAllowed", which needs to be changed.
>>> JR41.2 It started with "For users who configure an empty list", but not
>> all
>>> the bullet points are about empty lists.
>>> JR41.3 To me, it would be useful to summarize why the behavior changes
>> are
>>> justified. We made 3 types of changes. (1) Disallow null. This is ok
>> since
>>> one can't really set a null value through a config file. (2) Disallow
>>> duplicates. This is ok since it's rare and confusing. (3) Disallow empty.
>>> Ideally, we want to claim that for those cases, they all lead to some
>> other
>>> errors or had behavior currently.
>>>
>>> Jun
>>>
>>> On Wed, Jul 23, 2025 at 4:42 AM 黃竣陽 <s7133...@gmail.com> wrote:
>>>
>>>> Hello Jun,
>>>>
>>>> Thanks for the reply.
>>>>
>>>> JR34 ,JR36, JR39: I’ve updated the KIP to reflect these changes.
>>>>
>>>> JR38: I clarified the following configurations:
>>>> early.start.listeners: This config is optional and allows both null and
>>>> empty lists.
>>>> log.dirs and plugin.path: These are also optional, but only allow
>>>> null—empty lists are not permitted.
>>>>
>>>> Best Regards,
>>>> Jiunn-Yang
>>>>
>>>>> Jun Rao <j...@confluent.io.INVALID> 於 2025年7月23日 凌晨2:28 寫道：
>>>>>
>>>>> Hi, Jiunn-Yang and Chia-Ping,
>>>>>
>>>>> Thanks for the reply.
>>>>>
>>>>> JR34. Sounds good.
>>>>>
>>>>> JR36. Could you document that the default value for
>>>>> sasl.oauthbearer.expected.audience and ssl.cipher.suites have been
>>>> changed?
>>>>>
>>>>> JR38. My concern wasn't quite addressed. "If the configuration is
>>>> optional,
>>>>> we will reject any duplicate values in the list." This implies that if
>> a
>>>>> config is optional, we allow empty lists. However, for plugin.path, we
>>>>> reject empty lists.
>>>>>
>>>>> JR39. We can remove the fields isEmptyAllowed and isNullAllowed since
>>>> they
>>>>> are not public facing.
>>>>>
>>>>> Jun
>>>>>
>>>>>
>>>>> On Tue, Jul 22, 2025 at 4:26 AM 黃竣陽 <s7133...@gmail.com> wrote:
>>>>>
>>>>>> Hello Jun, Chia,
>>>>>>
>>>>>> Thanks for the reply,
>>>>>>
>>>>>> JR36: I’ve updated the following configurations:
>>>>>> - sasl.oauthbearer.expected.audience
>>>>>> - ssl.cipher.suites
>>>>>> - ssl.enabled.protocols
>>>>>> These now allow both empty and non-empty lists. Since the default
>> values
>>>>>> of
>>>>>> sasl.oauthbearer.expected.audience and ssl.cipher.suites were
>> previously
>>>>>> null,
>>>>>> I’ve updated their defaults to List.of() for consistency.
>>>>>>
>>>>>> JR37: A ClassCastException does not occur in MirrorClientConfig and it
>>>>>> should be none.
>>>>>> I’ve updated this in the KIP.
>>>>>>
>>>>>> JR38 & JR40: I’ve added a note below the table to indicate which
>>>>>> configurations require special attention.
>>>>>>
>>>>>> JR39: Yes, these properties are package-private.
>>>>>>
>>>>>> Best Regards,
>>>>>> Jiunn-Yang
>>>>>>
>>>>>>> Chia-Ping Tsai <chia7...@gmail.com> 於 2025年7月22日 下午6:04 寫道：
>>>>>>>
>>>>>>> JR34. Hmm, it seems that the code in taskConfigs() could return an
>>>> empty
>>>>>>> list for a task if knownSourceTopicPartitions is less than maxTasks,
>>>>>>> Chia-Ping?
>>>>>>>
>>>>>>>
>>>>>>> Yes, `MirrorSourceConnector#taskConfigs` could return an empty list,
>>>>>> which
>>>>>>> means no `MirrorSourceTask` can be created.
>>>>>>>
>>>>>>> By contrast, if any `MirrorSourceTask` is created, the configs used
>> by
>>>>>>> `MirrorSourceTask` must contain `task.assigned.partitions` with a
>>>>>> non-empty
>>>>>>> value.
>>>>>>>
>>>>>>> Jun Rao <j...@confluent.io.invalid> 於 2025年7月22日 週二 上午5:14寫道：
>>>>>>>
>>>>>>>> Hi, Jiunn-Yang and Chia-Ping,
>>>>>>>>
>>>>>>>> Thanks for the reply.
>>>>>>>>
>>>>>>>> JR34. Hmm, it seems that the code in taskConfigs() could return an
>>>> empty
>>>>>>>> list for a task if knownSourceTopicPartitions is less than maxTasks,
>>>>>>>> Chia-Ping?
>>>>>>>>
>>>>>>>> JR36. Sounds good. Could you update the KIP? Also, should we do the
>>>> same
>>>>>>>> for ssl.cipher.suites and sasl.oauthbearer.expected.audience?
>>>>>>>>
>>>>>>>> JR37. Is it true that MirrorClientConfig.bootstrap.servers returns
>>>>>>>> ClassCastException for null? It seems that null could be casted to
>> any
>>>>>>>> class.
>>>>>>>>
>>>>>>>> JR38. "If the configuration is optional, we will reject any
>> duplicate
>>>>>>>> values in the list." Could you clarify this? For example,
>> plugin.path
>>>> is
>>>>>>>> optional, but with a default value. We reject duplicates as well as
>>>>>> empty
>>>>>>>> lists.
>>>>>>>>
>>>>>>>> JR39. isEmptyAllowed and isNullAllowed are not public fields, right?
>>>>>> They
>>>>>>>> are only public in the anyNonDuplicateValues() method.
>>>>>>>>
>>>>>>>> JR40. It would be useful to mention that if cleanup.policy is empty
>>>> and
>>>>>>>> remote.storage.enable is true, the local log segments will be
>> cleaned
>>>>>> based
>>>>>>>> on log.local.retention.bytes and log.local.retention.ms.
>>>>>>>>
>>>>>>>> Jun
>>>>>>>>
>>>>>>>> On Fri, Jul 18, 2025 at 9:52 AM Chia-Ping Tsai <chia7...@gmail.com>
>>>>>> wrote:
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> JR34: For these two configurations, setting an empty list feels a
>>>> bit
>>>>>>>>>> unintuitive. If an empty list is
>>>>>>>>>> provided, the consumer will call the unsubscribe method, which
>>>> doesn't
>>>>>>>>>> seem appropriate given
>>>>>>>>>> the documentation states: "Topic-partitions assigned to this task
>> to
>>>>>>>>>> replicate."
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> agreed. Additionally, the source code [0] shows that it is not
>>>> intended
>>>>>>>> to
>>>>>>>>> generate empty tps for the source task
>>>>>>>>>
>>>>>>>>> [0]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>> https://github.com/apache/kafka/blob/9b542b6ea21e84677a9292f250fc25f8b4162e6f/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConnector.java#L202
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
