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