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