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