Hello Jun,
Thanks for the reply,
JR46: I have clarified the behavior for null and empty values, so users can
understand the reasoning behind disallowing empty lists.
JR48: I've updated the KIP accordingly.
Best Regards,
Jiunn-Yang
> Jun Rao <j...@confluent.io.INVALID> 於 2025年8月5日 凌晨12:45 寫道：
>
> Hi, Jiunn-Yang,
>
> Thanks for the reply.
>
> JR46. "Currently, if plugin.path is set to an empty list, no user-specified
> plugin locations are processed, but plugins from the parent classloader are
> still loaded." Could you add why the current behavior is bad for the users?
>
> JR48. If we want to preserve the current behavior, we should remove the
> following validation, right?
> if (validStrings.length == 0) {
> throw new IllegalArgumentException("Valid strings list cannot
> be empty for in validator");
>
> Jun
>
> On Fri, Aug 1, 2025 at 6:47 PM 黃竣陽 <s7133...@gmail.com> wrote:
>
>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
