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