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