Hi, Jun Thanks for the reply.
I think the following configs could use null as the default value and treat an "empty list" as an illegal value. `sasl.oauthbearer.expected.audience`, `ssl.cipher.suites`, `ssl.enabled.protocols`, and `log.dirs` Jun Rao <j...@confluent.io.invalid> 於 2025年7月11日 週五 上午6:59寫道： > Hi, Chia-Ping, Jiunn-Yang, > > Thanks for the reply. > > As you said, if ssl.cipher.suites is empty, intuitively this means that no > cipher is allowed. So, this should be an illegal config. It just happens > that our implementation treats it as if the config is not set (meaning the > value is null). So, it seems that a more intuitive convention is: if the > config is not set, it defaults to all available cipher suites. If it's > explicitly set, it can't be empty. > > Jun > > > > On Wed, Jul 9, 2025 at 8:27 AM 黃竣陽 <s7133...@gmail.com> wrote: > > > Hi Jun, Chia > > > > >> Also, it would be useful to add a compatibility column in the table. > If > > a > > >> value is formally disallowed, it would be useful to compare the old > and > > the > > >> new behavior (e.g. a different exception is thrown, etc). > > I’ve updated the table to show which exceptions will be thrown for each > > behavior. > > > > >>> I don't understand the point of "5.0". Could you please share more > > details? > > >>> It seems to me changing the type or default value should be fine in > > minor > > >>> release if we don't break the existing property files. > > I don't think this requires any compatibility considerations, so I’ve > > removed the Kafka 5.0 changes section. > > > > >>> `max.connections.per.ip.overrides` is parsed as "map" type, so using > > `LIST` > > >>> instead of `String` does not seems to make sense. > > I have removed the config from KIP > > > > Best Regards, > > Jiunn-Yang > > > > > Chia-Ping Tsai <chia7...@apache.org> 於 2025年7月9日 晚上10:26 寫道： > > > > > >> For ssl.cipher.suites, changing the default value from null to empty > is > > >> actually a breaking change. According to the doc, null means that all > > the > > >> available cipher suites are supported. I am thinking that we should > > >> continue to allow null as the default since it could represent a state > > >> different from empty. > > > > > > "Empty list" seems to be a weird case. It appears to signify that no > > cipher suites are accepted. However, in the codebase, an empty list is > > handled as if all available cipher suites are supported [0]. We should > not > > support the case of "no supported cipher suite," as it doesn't make > sense. > > > > > > In short, changing the default value from null to empty does not break > > the behavior. > > > > > > [0] > > > https://github.com/apache/kafka/blob/dabde76ebf105aaa945db60b7753331c83a8c989/clients/src/main/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactory.java#L139 > > > > > > On 2025/07/08 17:01:07 Jun Rao wrote: > > >> Hi, Jiunn-Yang, > > >> > > >> I agree with Chia-Ping. If we allow cleanup.policy to be empty, we > will > > >> just continue to allow it in all future releases. > > >> > > >> For ssl.cipher.suites, changing the default value from null to empty > is > > >> actually a breaking change. According to the doc, null means that all > > the > > >> available cipher suites are supported. I am thinking that we should > > >> continue to allow null as the default since it could represent a state > > >> different from empty. > > >> > > >> Also, it would be useful to add a compatibility column in the table. > If > > a > > >> value is formally disallowed, it would be useful to compare the old > and > > the > > >> new behavior (e.g. a different exception is thrown, etc). > > >> > > >> Thanks, > > >> > > >> Jun > > >> > > >> On Tue, Jul 8, 2025 at 8:36 AM Chia-Ping Tsai <chia7...@gmail.com> > > wrote: > > >> > > >>> hi Jiunn > > >>> > > >>> Thanks for the explanation. I have updated the KIP accordingly: in > > Kafka > > >>>> 4.x, cleanup.policy > > >>>> can be set to an empty list with a warning message emitted; however, > > in > > >>>> Kafka 5.0, setting an > > >>>> empty list for cleanup.policy will no longer be allowed. > > >>> > > >>> > > >>> If we agree to support "empty list", the approach of "none" could be > > >>> eliminated, right? > > >>> > > >>> I don't understand the point of "5.0". Could you please share more > > details? > > >>> It seems to me changing the type or default value should be fine in > > minor > > >>> release if we don't break the existing property files. > > >>> > > >>> `max.connections.per.ip.overrides` is parsed as "map" type, so using > > `LIST` > > >>> instead of `String` does not seems to make sense. > > >>> > > >>> Best, > > >>> Chia-Ping > > >>> > > >>> > > >>> > > >>> > > >>> 黃竣陽 <s7133...@gmail.com> 於 2025年7月8日 週二 下午11:25寫道： > > >>> > > >>>> Hi all, > > >>>> > > >>>>>> Null could make sense as the default value, but it just means that > > the > > >>>>>> config key is not set explicitly. > > >>>>>> Given that, it's probably simpler to just allow > > >>>>>> log.cleanup.policy to be empty and properly support it? > > >>>> Thanks for the explanation. I have updated the KIP accordingly: in > > Kafka > > >>>> 4.x, cleanup.policy > > >>>> can be set to an empty list with a warning message emitted; however, > > in > > >>>> Kafka 5.0, setting an > > >>>> empty list for cleanup.policy will no longer be allowed. > > >>>> > > >>>>> For another, there is an additional issue which could be handled by > > >>> this > > >>>>> KIP. Some configs have "string" type, but they are handled as a > LIST. > > >>> For > > >>>>> example, `early.start.listeners`, `security.providers`. Could we > > change > > >>>>> their type to LIST to benefit from this KIP? I mean to make them > > >>>>> non-nullable. > > >>>> I have also updated the KIP to include the tables detailing the > > changes > > >>>> for LIST-type configuration > > >>>> default values and the migration from STRING-type to LIST-type > > >>>> configurations. > > >>>> > > >>>> Best Regards, > > >>>> Jiunn-Yang > > >>>> > > >>>>> Chia-Ping Tsai <chia7...@gmail.com> 於 2025年7月8日 清晨7:06 寫道： > > >>>>> > > >>>>> hi all, > > >>>>> > > >>>>> JR11. There is an existing config interceptor.classes that allows > an > > >>>> empty > > >>>>>> list and it makes intuitive sense. So, it seems that it's ok to > > >>> support > > >>>>>> empty lists in general. Given that, it's probably simpler to just > > >>> allow > > >>>>>> log.cleanup.policy to be empty and properly support it? > > >>>>> > > >>>>> > > >>>>> yes, that is what I meant before. Additionally, the `LIST` type > > should > > >>>> not > > >>>>> accept "null". Instead, we should use an empty list as the default > > >>> value. > > >>>>> For example, `sasl.oauthbearer.expected.audience` should never has > > >>> "null" > > >>>>> value. That could eliminate the null check. > > >>>>> > > >>>>> For another, there is an additional issue which could be handled by > > >>> this > > >>>>> KIP. Some configs have "string" type, but they are handled as a > LIST. > > >>> For > > >>>>> example, `early.start.listeners`, `security.providers`. Could we > > change > > >>>>> their type to LIST to benefit from this KIP? I mean to make them > > >>>>> non-nullable. > > >>>>> > > >>>>> Best, > > >>>>> Chia-Ping > > >>>>> > > >>>>> > > >>>>> > > >>>>> Jun Rao <j...@confluent.io.invalid> 於 2025年7月8日 週二 上午5:56寫道： > > >>>>> > > >>>>>> Hi, Jiunn-Yang, > > >>>>>> > > >>>>>> Thanks for the reply. > > >>>>>> > > >>>>>> JR10. Regarding supporting the null value, I am not sure how one > > could > > >>>> set > > >>>>>> a null value in a config file. For example, if you set the > following > > >>> in > > >>>> a > > >>>>>> config file. > > >>>>>> configkey= > > >>>>>> The value of configkey will be an empty list, instead of null, > > right? > > >>>>>> > > >>>>>> Null could make sense as the default value, but it just means that > > the > > >>>>>> config key is not set explicitly. > > >>>>>> > > >>>>>> JR11. There is an existing config interceptor.classes that allows > an > > >>>> empty > > >>>>>> list and it makes intuitive sense. So, it seems that it's ok to > > >>> support > > >>>>>> empty lists in general. Given that, it's probably simpler to just > > >>> allow > > >>>>>> log.cleanup.policy to be empty and properly support it? > > >>>>>> > > >>>>>> Jun > > >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> On Fri, Jul 4, 2025 at 5:13 AM 黃竣陽 <s7133...@gmail.com> wrote: > > >>>>>> > > >>>>>>> Hello Jun, > > >>>>>>> > > >>>>>>> JR10: The sasl.oauthbearer.expected.audience is optional, that is > > >>>> right, > > >>>>>>> so I think > > >>>>>>> that we can allow null value, but not allow the empty > > >>>>>>> > > >>>>>>> JR11: In this KIP, the goal is to address configurations that > > should > > >>>> not > > >>>>>>> accept empty arrays. > > >>>>>>> We want to prevent users from passing in an empty list when it's > > not > > >>>>>>> valid. I believe there are two > > >>>>>>> main scenarios to consider: > > >>>>>>> > > >>>>>>> 1. Nullable but not empty: > > >>>>>>> The parameter allows either a null value or a non-empty list. In > > this > > >>>>>>> case, > > >>>>>>> if the user provides an empty list, we should throw a > > >>> ConfigException. > > >>>>>>> > > >>>>>>> 2. Non-null and non-empty only: The parameter must always > contain a > > >>>>>>> non-empty list — null is also > > >>>>>>> invalid. In this case, if the user provides either a null or an > > empty > > >>>>>>> list, we should throw a ConfigException. > > >>>>>>> > > >>>>>>> Therefore, I want to prevent scenarios where users can pass in an > > >>> empty > > >>>>>>> list. > > >>>>>>> > > >>>>>>> JR12: I’ve updated NonEmptyListValidator to include an allowNull > > >>>>>> property. > > >>>>>>> This allows us to specify > > >>>>>>> whether a configuration permits null values or not. > > >>>>>>> > > >>>>>>> JR13: I have updated the table > > >>>>>>> > > >>>>>>> > > >>>>>>>> Jun Rao <j...@confluent.io.INVALID> 於 2025年7月1日 清晨6:30 寫道： > > >>>>>>>> > > >>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>> > > >>>>>>>> Sorry for the late reply. A few more comments. > > >>>>>>>> > > >>>>>>>> JR10. The doc for sasl.oauthbearer.expected.audience says that > > it's > > >>>>>>>> optional. So an empty list seems to be valid. > > >>>>>>>> > > >>>>>>>> JR11. Thinking a bit more. If we are going to support empty > lists > > in > > >>>>>>>> general, it's probably more consistent to just allow > > >>>> log.cleanup.policy > > >>>>>>> to > > >>>>>>>> be empty as Luke suggested, instead of introducing a new option > > >>> None. > > >>>>>>>> > > >>>>>>>> JR12. NonEmptyListValidator allows the input to be null. It > seems > > >>> that > > >>>>>>>> only ssl.cipher.suites allows null. The remaining ones shouldn't > > be > > >>>>>> null. > > >>>>>>>> > > >>>>>>>> JR13. The table is very helpful. Could you also describe the > > old/new > > >>>>>>>> behavior for each config with an empty list or duplicate entry? > > >>>>>>>> > > >>>>>>>> Thanks, > > >>>>>>>> > > >>>>>>>> Jun > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> On Mon, Jun 30, 2025 at 6:02 AM 黃竣陽 <s7133...@gmail.com> wrote: > > >>>>>>>> > > >>>>>>>>> Hello all, > > >>>>>>>>> > > >>>>>>>>> I am manually bumping this thread. Any feedback would be > > >>> appreciated. > > >>>>>>>>> > > >>>>>>>>> Best regards, > > >>>>>>>>> Jiunn-Yang > > >>>>>>>>> > > >>>>>>>>>> 黃竣陽 <s7133...@gmail.com> 於 2025年5月28日 晚上11:21 寫道： > > >>>>>>>>>> > > >>>>>>>>>> Hello Jun, > > >>>>>>>>>> > > >>>>>>>>>> I have updated the KIP and introduced a new validator, > > >>>>>>>>> NonEmptyListValidator, which ensures that the > > >>>>>>>>>> provided list is either null or a non-empty list without > > duplicate > > >>>>>>>>> entries. I’ve also listed the configurations that will > > >>>>>>>>>> be validated using this logic. Please feel free to share any > > >>>> feedback > > >>>>>>> or > > >>>>>>>>> suggestions. > > >>>>>>>>>> > > >>>>>>>>>> Best Regards, > > >>>>>>>>>> Jiunn-Yang > > >>>>>>>>>> > > >>>>>>>>>>> Jun Rao <j...@confluent.io.INVALID> 於 2025年5月23日 凌晨2:29 寫道： > > >>>>>>>>>>> > > >>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>> > > >>>>>>>>>>> It seems there are quite a few other configs of type list > (e.g. > > >>>>>>> several > > >>>>>>>>> SSL > > >>>>>>>>>>> related ones). It would be useful to understand if empty > lists > > >>> are > > >>>>>>> valid > > >>>>>>>>>>> for them or not. > > >>>>>>>>>>> > > >>>>>>>>>>> Thanks, > > >>>>>>>>>>> > > >>>>>>>>>>> Jun > > >>>>>>>>>>> > > >>>>>>>>>>> On Tue, May 13, 2025 at 10:12 AM Jun Rao <j...@confluent.io> > > >>> wrote: > > >>>>>>>>>>> > > >>>>>>>>>>>> Hi, Luke, > > >>>>>>>>>>>> > > >>>>>>>>>>>> Thanks for the reply. > > >>>>>>>>>>>> > > >>>>>>>>>>>> "My thought is that this behavior has been existed > > >>>>>>>>>>>> for years (maybe after "cleanup.policy" was introduced?), > > there > > >>>>>> could > > >>>>>>>>> be > > >>>>>>>>>>>> users relying on the empty cleanup.policy for a long time. " > > >>>>>>>>>>>> > > >>>>>>>>>>>> If you do a google search on "kafka infinite retention", you > > >>> will > > >>>>>> get > > >>>>>>>>>>>> links like > > >>>>>>>>>>>> > > >>>>>>>>> > > >>>>>>> > > >>>>>> > > >>>> > > >>> > > > https://stackoverflow.com/questions/39735036/make-kafka-topic-log-retention-permanent > > >>>>>>>>>>>> and > > >>>>>>>>>>>> > > >>>>>>>>> > > >>>>>>> > > >>>>>> > > >>>> > > >>> > > > https://www.reddit.com/r/apachekafka/comments/mpz4bp/kafka_retention_question/ > > >>>>>>>>> , > > >>>>>>>>>>>> both pointing to setting the retention time and retention > size > > >>> to > > >>>>>> -1 > > >>>>>>> to > > >>>>>>>>>>>> achieve this goal. So, it seems that if a user intentionally > > >>> wants > > >>>>>>>>> infinite > > >>>>>>>>>>>> retention, it's more likely they will use that approach > > instead > > >>> of > > >>>>>>>>> setting > > >>>>>>>>>>>> cleanup.policy to empty. On the other hand, our API/tools > make > > >>> it > > >>>>>>>>> possible > > >>>>>>>>>>>> for users to make a mistake by inadvertently leaving the > > config > > >>>>>> value > > >>>>>>>>> empty. > > >>>>>>>>>>>> > > >>>>>>>>>>>> Thanks, > > >>>>>>>>>>>> > > >>>>>>>>>>>> Jun > > >>>>>>>>>>>> > > >>>>>>>>>>>> On Mon, May 12, 2025 at 11:38 PM Luke Chen < > show...@gmail.com > > > > > >>>>>>> wrote: > > >>>>>>>>>>>> > > >>>>>>>>>>>>> Hi Jun, > > >>>>>>>>>>>>> > > >>>>>>>>>>>>>> Regarding the motivation, currently, we never documented > > that > > >>> an > > >>>>>>>>> empty > > >>>>>>>>>>>>> cleanup.policy implies infinite retention. In fact, if one > > >>> leaves > > >>>>>>>>>>>>> cleanup.policy empty, it actually breaks remote storage > since > > >>> it > > >>>>>>> will > > >>>>>>>>> stop > > >>>>>>>>>>>>> the cleaning based on local retention size and time too. > So, > > >>>>>> leaving > > >>>>>>>>>>>>> cleanup.policy empty is probably more like a user mistake > > than > > >>> an > > >>>>>>>>>>>>> intentional choice. Based on this assumption, the idea > behind > > >>> the > > >>>>>>> KIP > > >>>>>>>>> is > > >>>>>>>>>>>>> to > > >>>>>>>>>>>>> fail an empty cleanup.policy so that the user is aware of > > this > > >>>>>>> likely > > >>>>>>>>>>>>> mistake and provide a new option None for users wanting > > >>> infinite > > >>>>>>>>> retention > > >>>>>>>>>>>>> to opt in explicitly. > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> While we never documented the empty cleanup.policy implies > > >>>>>> infinite > > >>>>>>>>>>>>> retention, we also never documented (or failed) the empty > > >>>>>>>>> cleanup.policy > > >>>>>>>>>>>>> is > > >>>>>>>>>>>>> an invalid configuration. My thought is that this behavior > > has > > >>>>>> been > > >>>>>>>>>>>>> existed > > >>>>>>>>>>>>> for years (maybe after "cleanup.policy" was introduced?), > > there > > >>>>>>> could > > >>>>>>>>> be > > >>>>>>>>>>>>> users relying on the empty cleanup.policy for a long time. > It > > >>> is > > >>>>>> not > > >>>>>>>>> good > > >>>>>>>>>>>>> to break the backward compatibility in a minor release > > version > > >>>>>> (ex: > > >>>>>>>>>>>>> v4.1.0). Even though we think we are fixing a long existing > > >>> bug, > > >>>>>> it > > >>>>>>>>> could > > >>>>>>>>>>>>> be a surprise to users. > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>>>> We could also consider supporting an empty cleanup.policy > by > > >>>>>> fixing > > >>>>>>>>> the > > >>>>>>>>>>>>> issue in remote storage. But then the user may never > realize > > >>> this > > >>>>>> if > > >>>>>>>>> it's > > >>>>>>>>>>>>> a > > >>>>>>>>>>>>> mistake. > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> Good point! I agree we need to fix it! > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> Hi Chia-Ping, > > >>>>>>>>>>>>> > > >>>>>>>>>>>>>> We can print warnings for empty cleanup.policy in 4.x and > in > > >>> 5.0 > > >>>>>> we > > >>>>>>>>>>>>> throw > > >>>>>>>>>>>>> exception directly to make it fail fast > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> This suggestion looks good to me! > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> Thank you. > > >>>>>>>>>>>>> Luke > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> On Tue, May 13, 2025 at 2:14 PM Chia-Ping Tsai < > > >>>>>> chia7...@apache.org > > >>>>>>>> > > >>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>> > > >>>>>>>>>>>>>> hi all, > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> Given Luke mentioned the existence of use cases, it is > worth > > >>>>>>>>> considering > > >>>>>>>>>>>>>> the compatibility issue. In fact, I had previously > > encountered > > >>>>>> this > > >>>>>>>>> use > > >>>>>>>>>>>>>> case but advised customers to utilize retention > > configurations > > >>>>>>>>> instead. > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> We could also consider supporting an empty cleanup.policy > > by > > >>>>>>> fixing > > >>>>>>>>>>>>> the > > >>>>>>>>>>>>>>> issue in remote storage. But then the user may never > > realize > > >>>>>> this > > >>>>>>> if > > >>>>>>>>>>>>>> it's a > > >>>>>>>>>>>>>>> mistake. > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> Good catch! The "empty" or "none" wouldn't make sense for > > >>> remote > > >>>>>>>>>>>>> storage. > > >>>>>>>>>>>>>> I've opened KAFKA-19273 to ensure topics with tiered > storage > > >>> use > > >>>>>> a > > >>>>>>>>> valid > > >>>>>>>>>>>>>> delete policy. > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> Best, > > >>>>>>>>>>>>>> Chia-Ping > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> On 2025/05/12 19:06:10 Jun Rao wrote: > > >>>>>>>>>>>>>>> Hi, Luke, > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> Regarding the motivation, currently, we never documented > > that > > >>>> an > > >>>>>>>>> empty > > >>>>>>>>>>>>>>> cleanup.policy implies infinite retention. In fact, if > one > > >>>>>> leaves > > >>>>>>>>>>>>>>> cleanup.policy empty, it actually breaks remote storage > > since > > >>>> it > > >>>>>>>>> will > > >>>>>>>>>>>>>> stop > > >>>>>>>>>>>>>>> the cleaning based on local retention size and time too. > > So, > > >>>>>>> leaving > > >>>>>>>>>>>>>>> cleanup.policy empty is probably more like a user mistake > > >>> than > > >>>>>> an > > >>>>>>>>>>>>>>> intentional choice. Based on this assumption, the idea > > behind > > >>>>>> the > > >>>>>>>>> KIP > > >>>>>>>>>>>>> is > > >>>>>>>>>>>>>> to > > >>>>>>>>>>>>>>> fail an empty cleanup.policy so that the user is aware of > > >>> this > > >>>>>>>>> likely > > >>>>>>>>>>>>>>> mistake and provide a new option None for users wanting > > >>>> infinite > > >>>>>>>>>>>>>> retention > > >>>>>>>>>>>>>>> to opt in explicitly. > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> We could also consider supporting an empty cleanup.policy > > by > > >>>>>>> fixing > > >>>>>>>>>>>>> the > > >>>>>>>>>>>>>>> issue in remote storage. But then the user may never > > realize > > >>>>>> this > > >>>>>>> if > > >>>>>>>>>>>>>> it's a > > >>>>>>>>>>>>>>> mistake. > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> On Sun, May 11, 2025 at 7:53 PM Luke Chen < > > show...@gmail.com > > >>>> > > >>>>>>>>> wrote: > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> Hi Jiunn-Yang, > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> Thanks for the KIP. > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> Comments: > > >>>>>>>>>>>>>>>> 1. "it is acceptable because an empty cleanup.policy is > > >>>>>>> considered > > >>>>>>>>>>>>>> invalid > > >>>>>>>>>>>>>>>> in Kafka. Therefore, this trade-off is justified." > > >>>>>>>>>>>>>>>> Could you explain more about this? Why is this trade-off > > >>>>>>> justified? > > >>>>>>>>>>>>>>>> If we think the empty cleanup.policy is invalid in > kafka, > > >>> why > > >>>>>> do > > >>>>>>> we > > >>>>>>>>>>>>>> provide > > >>>>>>>>>>>>>>>> an additional "none" option to users in this KIP? > > >>>>>>>>>>>>>>>> FYI, there are indeed use cases that need to keep all > > >>> history > > >>>>>>> data > > >>>>>>>>>>>>>> without > > >>>>>>>>>>>>>>>> a cleanup policy. > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> 2. Following (1), I agree we should fail fast for empty > > >>>>>>>>>>>>>>>> "group.coordinator.rebalance.protocols" and > > "process.roles" > > >>>>>>> configs > > >>>>>>>>>>>>>> since > > >>>>>>>>>>>>>>>> they are invalid. > > >>>>>>>>>>>>>>>> But about "cleanup.policy", I don't see a persuasive > > reason > > >>>> why > > >>>>>>> we > > >>>>>>>>>>>>>> should > > >>>>>>>>>>>>>>>> break backward compatibility to change it. > > >>>>>>>>>>>>>>>> "This provides a clear and direct way to configure Kafka > > to > > >>>>>>> retain > > >>>>>>>>>>>>> all > > >>>>>>>>>>>>>> log > > >>>>>>>>>>>>>>>> segments without any automatic cleanup." > > >>>>>>>>>>>>>>>> This is the motivation we mentioned in the KIP, but to > me, > > >>>>>>> backward > > >>>>>>>>>>>>>>>> compatibility is much more important than "a clear and > > >>> direct > > >>>>>> way > > >>>>>>>>> to > > >>>>>>>>>>>>>> config > > >>>>>>>>>>>>>>>> kafka". > > >>>>>>>>>>>>>>>> Do we really have to change the "cleanup.policy" config? > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> Thanks. > > >>>>>>>>>>>>>>>> Luke > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> On Wed, May 7, 2025 at 2:17 AM Jun Rao > > >>>>>> <j...@confluent.io.invalid > > >>>>>>>> > > >>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> Thanks for the updated KIP. It looks good to me. > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> On Tue, May 6, 2025 at 4:58 AM 黃竣陽 <s7133...@gmail.com > > > > >>>>>> wrote: > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> Hi Jun, chia > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> Thanks for all the comments. They have all been > > addressed > > >>> in > > >>>>>>> the > > >>>>>>>>>>>>>>>> updated > > >>>>>>>>>>>>>>>>>> KIP. > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> I've removed all deprecated-related sections. > > >>> Additionally, > > >>>>>> an > > >>>>>>>>>>>>>>>> exception > > >>>>>>>>>>>>>>>>>> will now be thrown if a > > >>>>>>>>>>>>>>>>>> developer passes an empty validStrings list to the > > >>>> inNonEmpty > > >>>>>>>>>>>>>> method. > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> Best Regards, > > >>>>>>>>>>>>>>>>>> Jiunn-Yang > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> Chia-Ping Tsai <chia7...@gmail.com> 於 2025年5月6日 > > 凌晨12:46 > > >>>> 寫道： > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> hi Jiunn-Yang > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> The `inNonEmpty` is used to prevent users pass empty > > >>>> config, > > >>>>>>>>>>>>> so > > >>>>>>>>>>>>>>>> should > > >>>>>>>>>>>>>>>>> it > > >>>>>>>>>>>>>>>>>>> be non-empty too? > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> For example, `inNonEmpty()` should throw exception > > >>>> directly. > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> Best, > > >>>>>>>>>>>>>>>>>>> Chia-Ping > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> Jun Rao <j...@confluent.io.invalid> 於 2025年5月6日 週二 > > >>>>>> 上午12:28寫道： > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> Thanks for the reply. There are still references to > > the > > >>>>>>>>>>>>>> deprecated > > >>>>>>>>>>>>>>>>>> method. > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> "stop relying on the deprecated methods" > > >>>>>>>>>>>>>>>>>>>> "Finally, the deprecated method will be removed in > > Kafka > > >>>>>> 5.0" > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> On Sat, May 3, 2025 at 7:42 AM 黃竣陽 < > > s7133...@gmail.com> > > >>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>> Hi Jun, chia, > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>> Thanks for all the comments. They have all been > > >>> addressed > > >>>>>> in > > >>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>> updated > > >>>>>>>>>>>>>>>>>>>>> KIP. > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>> Best Regards, > > >>>>>>>>>>>>>>>>>>>>> Jiunn-Yang > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> Chia-Ping Tsai <chia7...@gmail.com> 於 2025年5月3日 > > >>> 凌晨2:03 > > >>>>>> 寫道： > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> hi Jiunn-Yang > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> in the "Compatibility, Deprecation, and Migration > > >>> Plan", > > >>>>>>>>>>>>>> could you > > >>>>>>>>>>>>>>>>> add > > >>>>>>>>>>>>>>>>>>>>>> description to remind readers that "clean.policy=" > > >>>> should > > >>>>>>>>>>>>> be > > >>>>>>>>>>>>>>>>> replaced > > >>>>>>>>>>>>>>>>>>>> by > > >>>>>>>>>>>>>>>>>>>>>> "clean.policy=none" if they do want to disable > > delete > > >>>> and > > >>>>>>>>>>>>>> compact. > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> Best, > > >>>>>>>>>>>>>>>>>>>>>> Chia-Ping > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> Jun Rao <j...@confluent.io.invalid> 於 2025年5月3日 週六 > > >>>>>>>>>>>>> 上午1:55寫道： > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> It's fine not to deprecate ValidList#in for now. > > >>> Could > > >>>>>> you > > >>>>>>>>>>>>>> update > > >>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>> KIP > > >>>>>>>>>>>>>>>>>>>>>>> completely? There are still references to > > >>> deprecation. > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> On Fri, May 2, 2025 at 4:59 AM 黃竣陽 < > > >>> s7133...@gmail.com > > >>>>> > > >>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> Hi Jun, > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> I don’t think we should deprecate the > ValidList#in > > >>>>>>>>>>>>> method, > > >>>>>>>>>>>>>> as > > >>>>>>>>>>>>>>>>> there > > >>>>>>>>>>>>>>>>>>>> may > > >>>>>>>>>>>>>>>>>>>>>>> be > > >>>>>>>>>>>>>>>>>>>>>>>> future scenarios where we want > > >>>>>>>>>>>>>>>>>>>>>>>> to allow list configs to be empty. It’s useful > to > > >>> have > > >>>>>>>>>>>>> both > > >>>>>>>>>>>>>>>>> methods: > > >>>>>>>>>>>>>>>>>>>> in > > >>>>>>>>>>>>>>>>>>>>>>>> (which allows empty lists) > > >>>>>>>>>>>>>>>>>>>>>>>> and inNonEmpty (which enforces non-empty lists). > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>> Just a minor comment. It would be useful to > > >>> document > > >>>>>>>>>>>>> that > > >>>>>>>>>>>>>>>> during > > >>>>>>>>>>>>>>>>>>>>>>> upgrade, > > >>>>>>>>>>>>>>>>>>>>>>>>> if cleanup.policy is empty, the broker will > fail > > to > > >>>>>>>>>>>>> start. > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> I’ve updated the KIP accordingly. > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> Best Regards, > > >>>>>>>>>>>>>>>>>>>>>>>> Jiunn-Yang > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>> Jun Rao <j...@confluent.io.INVALID> 於 2025年5月2日 > > >>>>>> 凌晨12:50 > > >>>>>>>>>>>>> 寫道： > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>> Thanks for the reply. Sounds good. > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>> Just a minor comment. It would be useful to > > >>> document > > >>>>>>>>>>>>> that > > >>>>>>>>>>>>>>>> during > > >>>>>>>>>>>>>>>>>>>>>>> upgrade, > > >>>>>>>>>>>>>>>>>>>>>>>>> if cleanup.policy is empty, the broker will > fail > > to > > >>>>>>>>>>>>> start. > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>> On Thu, May 1, 2025 at 8:40 AM 黃竣陽 < > > >>>>>> s7133...@gmail.com> > > >>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> Hello Jun, > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> Since ValidList is a public API, we need to > > >>> maintain > > >>>>>>>>>>>>>> backward > > >>>>>>>>>>>>>>>>>>>>>>>>>> compatibility. Therefore, the isEmptyAllowed > > flag > > >>> is > > >>>>>>>>>>>>>>>> necessary. > > >>>>>>>>>>>>>>>>>>>>>>>>>> We can update the signature of isNonEmpty() to > > >>>> remove > > >>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>> boolean > > >>>>>>>>>>>>>>>>>>>>>>>>>> parameter. > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> Best Regards, > > >>>>>>>>>>>>>>>>>>>>>>>>>> Jiunn-Yang > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Jun Rao <j...@confluent.io.INVALID> 於 > 2025年5月1日 > > >>>>>> 凌晨4:17 > > >>>>>>>>>>>>>> 寫道： > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for the reply. > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Do we really need isEmptyAllowed? It's > awkward > > >>>> since > > >>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>> method > > >>>>>>>>>>>>>>>>>>>> name > > >>>>>>>>>>>>>>>>>>>>>>> is > > >>>>>>>>>>>>>>>>>>>>>>>>>>> inNonEmpty. Also, it's not clear when it's > set > > to > > >>>>>>>>>>>>> true. > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> On Fri, Apr 25, 2025 at 6:26 AM 黃竣陽 < > > >>>>>>>>>>>>> s7133...@gmail.com> > > >>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Jun, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> Thank you for pointing that out. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> I have revised the KIP as follows: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> “In this case, the change does not introduce > > new > > >>>>>>>>>>>>> invalid > > >>>>>>>>>>>>>>>>>>>>>>>> configurations > > >>>>>>>>>>>>>>>>>>>>>>>>>> or > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> prevent any currently valid setup. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> The main behavioral difference is that we > now > > >>>>>>>>>>>>> explicitly > > >>>>>>>>>>>>>>>>> throw a > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> ConfigException during the storage format > > >>>>>> validation > > >>>>>>>>>>>>>> phase > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> instead of relying on the current behavior.” > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> This reflects the correct behavior. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> Best Regards, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> Jiunn-Yang > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jun Rao <j...@confluent.io.INVALID> 於 > > >>> 2025年4月25日 > > >>>>>>>>>>>>>> 凌晨1:17 寫道： > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> "The main behavioral difference introduced > by > > >>>> this > > >>>>>>>>>>>>>> change > > >>>>>>>>>>>>>>>> is > > >>>>>>>>>>>>>>>>>>>> that > > >>>>>>>>>>>>>>>>>>>>> a > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> ConfigException will now be thrown during > > the > > >>>>>>>>>>>>> storage > > >>>>>>>>>>>>>>>> format > > >>>>>>>>>>>>>>>>>>>>>>>>>> validation > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> phase, rather than during server startup." > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> It seems that currently formating the > storage > > >>>>>>>>>>>>> already > > >>>>>>>>>>>>>> fails > > >>>>>>>>>>>>>>>>> if > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> group.coordinator.rebalance.protocols and > > >>>>>>>>>>>>> process.roles > > >>>>>>>>>>>>>>>> are > > >>>>>>>>>>>>>>>>>>>>> empty. > > >>>>>>>>>>>>>>>>>>>>>>>> It > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> just > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> gets a different exception. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Thu, Apr 24, 2025 at 5:32 AM 黃竣陽 < > > >>>>>>>>>>>>>> s7133...@gmail.com> > > >>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Jun, chia, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for your feedback. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I add a new section for this change: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - Validation for > > >>>>>>>>>>>>>> group.coordinator.rebalance.protocols and > > >>>>>>>>>>>>>>>>>>>>>>>>>> process.roles > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> will be > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> moved from the server startup phase to the > > >>>>>> storage > > >>>>>>>>>>>>>> format > > >>>>>>>>>>>>>>>>>>>> phase. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - For cleanup.policy, setting an empty > list > > >>> will > > >>>>>>>>>>>>> now > > >>>>>>>>>>>>>> be > > >>>>>>>>>>>>>>>>>>>>> considered > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> invalid > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and will > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> result in a ConfigException during storage > > >>>> format > > >>>>>>>>>>>>>> phase. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Best Regards, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jiunn-Yang > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jun Rao <j...@confluent.io.INVALID> 於 > > >>>> 2025年4月24日 > > >>>>>>>>>>>>>> 凌晨4:44 > > >>>>>>>>>>>>>>>> 寫道： > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for the reply. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Q2. It's true that > > >>>>>>>>>>>>>> group.coordinator.rebalance.protocols > > >>>>>>>>>>>>>>>>> and > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> process.roles > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> configurations can't be empty right now. > > With > > >>>>>> this > > >>>>>>>>>>>>>> KIP, > > >>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>> user > > >>>>>>>>>>>>>>>>>>>>>>>> will > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> probably get a different (and more > > accurate) > > >>>>>>>>>>>>>> exception. > > >>>>>>>>>>>>>>>> It > > >>>>>>>>>>>>>>>>>>>> will > > >>>>>>>>>>>>>>>>>>>>>>> be > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> useful > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to document that. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Regarding cleanup.policy, I kind of agree > > >>> with > > >>>>>>>>>>>>>> Chia-Ping. > > >>>>>>>>>>>>>>>>> If > > >>>>>>>>>>>>>>>>>> a > > >>>>>>>>>>>>>>>>>>>>>>> user > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> leaves > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> cleanup.policy empty, it's more likely to > > be > > >>> a > > >>>>>>>>>>>>>> mistake > > >>>>>>>>>>>>>>>> than > > >>>>>>>>>>>>>>>>>> an > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> intention. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> If we automatically treat it as None, the > > >>> user > > >>>>>>>>>>>>> will > > >>>>>>>>>>>>>> never > > >>>>>>>>>>>>>>>>>>>>> realize > > >>>>>>>>>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mistake. Throwing an exception will let > the > > >>>> user > > >>>>>>>>>>>>>> realize > > >>>>>>>>>>>>>>>>>> there > > >>>>>>>>>>>>>>>>>>>>>>> is a > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mistake. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Apr 23, 2025 at 8:26 AM Chia-Ping > > >>> Tsai > > >>>> < > > >>>>>>>>>>>>>>>>>>>>>>> chia7...@gmail.com > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> hi Jiunn-Yang, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for the KIP, and I understand > that > > >>>>>>>>>>>>>> maintaining > > >>>>>>>>>>>>>>>>>>>>>>>> compatibility > > >>>>>>>>>>>>>>>>>>>>>>>>>> is > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> important. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> However, according to the documentation, > > we > > >>>>>> don't > > >>>>>>>>>>>>>> allow > > >>>>>>>>>>>>>>>> an > > >>>>>>>>>>>>>>>>>>>>> empty > > >>>>>>>>>>>>>>>>>>>>>>>>>> value > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> for > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the cleanup.policy. Hence, should we > > >>> consider > > >>>>>>>>>>>>>> throwing > > >>>>>>>>>>>>>>>> an > > >>>>>>>>>>>>>>>>>>>>>>>> exception > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> for > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> an > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> empty value in version 4.x? This could > > >>>>>> streamline > > >>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>> code > > >>>>>>>>>>>>>>>>>>>> and > > >>>>>>>>>>>>>>>>>>>>>>>> avoid > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> an > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> extra deprecation cycle. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Best, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Chia-Ping > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jun Rao <j...@confluent.io.invalid> 於 > > >>>>>> 2025年4月23日 > > >>>>>>>>>>>>> 週三 > > >>>>>>>>>>>>>>>>>> 上午6:33寫道： > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Regarding "The none policy will not > > delete > > >>> or > > >>>>>>>>>>>>>> compact > > >>>>>>>>>>>>>>>> any > > >>>>>>>>>>>>>>>>>>>>>>>>>> segments", > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> we > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> should be more accurate. We won't > delete > > >>>>>>>>>>>>> segments > > >>>>>>>>>>>>>> based > > >>>>>>>>>>>>>>>>> on > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> log.retention.bytes/log.retention.ms, > > but > > >>> we > > >>>>>>>>>>>>>> should > > >>>>>>>>>>>>>>>>>>>> continue > > >>>>>>>>>>>>>>>>>>>>>>> to > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> delete > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> segments based on > > >>> log.local.retention.bytes/ > > >>>>>>>>>>>>>>>>>>>> log.retention.ms. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Otherwise, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> we > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> risk running out of local disk space > when > > >>>>>> remote > > >>>>>>>>>>>>>>>> storage > > >>>>>>>>>>>>>>>>> is > > >>>>>>>>>>>>>>>>>>>>>>>>>> enabled. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, Apr 22, 2025 at 9:45 AM Jun > Rao < > > >>>>>>>>>>>>>>>>> j...@confluent.io> > > >>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for the reply. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Q2. What about existing empty values > for > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> group.coordinator.rebalance.protocols > > and > > >>>>>>>>>>>>>>>> process.roles > > >>>>>>>>>>>>>>>>>>>>> during > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> upgrade? > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, Apr 22, 2025 at 7:29 AM 黃竣陽 < > > >>>>>>>>>>>>>>>> s7133...@gmail.com > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hello Jun, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for review this KIP. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Q1 & Q3: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I’ve updated the method name > > accordingly > > >>>> and > > >>>>>>>>>>>>>> revised > > >>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> cleanup.policy > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> documentation > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to clarify that the none policy > cannot > > be > > >>>>>> used > > >>>>>>>>>>>>>> with > > >>>>>>>>>>>>>>>> any > > >>>>>>>>>>>>>>>>>>>>> other > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> policy. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Q2: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> For users currently using an empty > > >>>>>>>>>>>>>> cleanup.policy, > > >>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>> approach > > >>>>>>>>>>>>>>>>>>>>>>>>>> is > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> to > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> apply the none policy > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> during the preProcessParsedConfig > step. > > >>>>>>>>>>>>>>>> Additionally, a > > >>>>>>>>>>>>>>>>>>>>>>> warning > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> message > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> will be emitted to inform users > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> of the upcoming change. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Best Regards, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jiunn-Yang > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jun Rao <j...@confluent.io.INVALID> > 於 > > >>>>>>>>>>>>> 2025年4月22日 > > >>>>>>>>>>>>>>>>> 凌晨4:52 > > >>>>>>>>>>>>>>>>>>>> 寫道： > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, Jiunn-Yang, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for the KIP. A few comments. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1. It's fine to introduce a new > value > > >>> None > > >>>>>>>>>>>>> for > > >>>>>>>>>>>>>>>>>>>>>>> cleanup.policy. > > >>>>>>>>>>>>>>>>>>>>>>>>>> But > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> now > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> not > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> all value combinations are valid. > For > > >>>>>>>>>>>>> example, > > >>>>>>>>>>>>>> None > > >>>>>>>>>>>>>>>>>> can't > > >>>>>>>>>>>>>>>>>>>>> be > > >>>>>>>>>>>>>>>>>>>>>>>>>> used > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> with > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Delete or Compact. It would be > useful > > to > > >>>>>>>>>>>>>> document > > >>>>>>>>>>>>>>>>> that. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 2. What's the behavior during > upgrade > > >>> when > > >>>>>> an > > >>>>>>>>>>>>>>>> existing > > >>>>>>>>>>>>>>>>>>>>>>> config > > >>>>>>>>>>>>>>>>>>>>>>>>>> has > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> an > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> empty > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> list. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3. inWithEmptyCheck: It's not clear > > what > > >>>>>> the > > >>>>>>>>>>>>>> empty > > >>>>>>>>>>>>>>>>> check > > >>>>>>>>>>>>>>>>>>>>>>> does. > > >>>>>>>>>>>>>>>>>>>>>>>>>> How > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> about > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> sth like inNonEmpty ? > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jun > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, Apr 15, 2025 at 8:25 AM 黃竣陽 > < > > >>>>>>>>>>>>>>>>> s7133...@gmail.com > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hello everyone, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I would like to start a discussion > on > > >>>>>>>>>>>>> KIP-1161: > > >>>>>>>>>>>>>>>>>>>>>>>> cleanup.policy > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> shouldn't > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> be empty > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> < > > >>>>>>>>>>>>> https://cwiki.apache.org/confluence/x/HArXF> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This proposal aims to improve the > > >>>>>>>>>>>>>> cleanup.policy > > >>>>>>>>>>>>>>>>>>>>>>>> configuration. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Currently, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> this setting should not be left > > empty. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Therefore, there are two proposed > > >>>>>>>>>>>>> improvements: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1. Update ValidList to validate > > whether > > >>>> an > > >>>>>>>>>>>>>> empty > > >>>>>>>>>>>>>>>> list > > >>>>>>>>>>>>>>>>>> is > > >>>>>>>>>>>>>>>>>>>>>>>>>> allowed. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 2. Introduce a new 'none' value for > > >>>>>>>>>>>>>> cleanup.policy. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Best Regards, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jiunn-Yang > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>> > > >>>> > > >>>> > > >>>> > > >>> > > >> > > > > > > >
