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
> >>>>>>>
> >>>>>>
> >>>>
> >>>>
> >>
> >>
>
>
