Hi Luke, Chia, Thanks for your information.
>> FYI, there are indeed use cases that need to keep all history data without >> a cleanup policy. I agree that we should maintain backward compatibility. Therefore, I will update the KIP as follows: - If the user sets an empty cleanup.policy, we will log a warning message and default it to 'none’. - Starting from Kafka 5.0, setting an empty cleanup.policy will no longer be allowed. Best Regards, Jiunn-Yang > Chia-Ping Tsai <chia7...@gmail.com> 於 2025年5月12日 上午11:56 寫道： > > hi Luke > > We can print warnings for empty cleanup.policy in 4.x and in 5.0 we throw > exception directly to make it fail fast > > Jiunn: WDYT? > > Best, > Chia-Ping > >> Luke Chen <show...@gmail.com> 於 2025年5月12日 上午10:52 寫道： >> >> 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 >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>> >>>> >>>
