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