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