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