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