hi Jiunn-Yang The `inNonEmpty` is used to prevent users pass empty config, so should it be non-empty too?
For example, `inNonEmpty()` should throw exception directly. Best, Chia-Ping Jun Rao <j...@confluent.io.invalid> 於 2025年5月6日 週二 上午12:28寫道： > Hi, Jiunn-Yang, > > Thanks for the reply. There are still references to the deprecated method. > > "stop relying on the deprecated methods" > "Finally, the deprecated method will be removed in Kafka 5.0" > > Thanks, > > Jun > > On Sat, May 3, 2025 at 7:42 AM 黃竣陽 <s7133...@gmail.com> wrote: > > > Hi Jun, chia, > > > > Thanks for all the comments. They have all been addressed in the updated > > KIP. > > > > Best Regards, > > Jiunn-Yang > > > > > Chia-Ping Tsai <chia7...@gmail.com> 於 2025年5月3日 凌晨2:03 寫道： > > > > > > 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 > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>> > > >>>>> > > >>> > > >>> > > >> > > > > >
