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