hi Jiunn-Yang
chia05:
`task.assigned.groups` and `task.assigned.partitions` should not have
default value, right?
chia06:
`new HashSet<>(values).size()` -> `Set.copyOf(values).size()`
Best,
Chia-Ping
Jun Rao <j...@confluent.io.invalid> 於 2025年8月5日 週二 下午11:39寫道：
> 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
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>
> > >>>>
> > >>
> > >>
> >
> >
>
