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