Hi, Jiunn-Yang,
Thanks for the reply and your patience. The KIP looks good to me now.
Hi, Luke, Mickael,
Could you take a look at the KIP again and see if it makes sense to you?
Thanks,
Jun
On Tue, Aug 5, 2025 at 3:04 AM 黃竣陽 <s7133...@gmail.com> wrote:
> 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
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>>>
> >>
> >>
>
>
