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