Hi, Jiunn-Yang, Thanks for the updated KIP. It looks good to me.
Jun On Tue, May 6, 2025 at 4:58 AM 黃竣陽 <s7133...@gmail.com> wrote: > Hi Jun, chia > > Thanks for all the comments. They have all been addressed in the updated > KIP. > > I've removed all deprecated-related sections. Additionally, an exception > will now be thrown if a > developer passes an empty validStrings list to the inNonEmpty method. > > Best Regards, > Jiunn-Yang > > > Chia-Ping Tsai <chia7...@gmail.com> 於 2025年5月6日 凌晨12:46 寫道： > > > > 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 > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>> > >>>>>> > >>>>> > >>> > >>> > >> > >
