Hi, Thanks for the KIP.
One weirdness of cleanup.policy is that you can set it to "delete,delete,delete,compact,compact" for example. Setting delete or compact multiple times has no functional impact. I don't think it's something that has to be addressed, but while you're looking at the configuration's validation I thought I'd bring it up. Thanks, Mickael On Mon, May 12, 2025 at 5:18 PM 黃竣陽 <s7133...@gmail.com> wrote: > > 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 > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>> > >>>>>>> > >>>>>> > >>>> > >>>> > >>> >
