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