hi Jiunn-Yang in the "Compatibility, Deprecation, and Migration Plan", could you add description to remind readers that "clean.policy=" should be replaced by "clean.policy=none" if they do want to disable delete and compact.
Best, Chia-Ping Jun Rao <j...@confluent.io.invalid> 於 2025年5月3日 週六 上午1:55寫道： > 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 > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>> > > >>>>>>>> > > >>>>>> > > >>>>>> > > >>>> > > >>>> > > >> > > >> > > > > >
