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