Hello Luke, thank you for finding this error, I have rectified it, and I will start a vote process soon.
-- Best, Ziming > On Jan 12, 2024, at 16:32, Luke Chen <show...@gmail.com> wrote: > > Hi Ziming, > > Thanks for the KIP! > LGTM! > Using incremental by defaul and fallback automatically if it's not > supported is a good idea! > > One minor comment: > 1. so I'm inclined to move it to incrementalAlterConfigs and "provide a > flag" to still use alterConfigs for new client to interact with old > servers. > I don't think we will "provide any flag" after the discussion. We should > remove it. > > Thanks. > Luke > > On Fri, Jan 12, 2024 at 12:29 PM ziming deng <dengziming1...@gmail.com > <mailto:dengziming1...@gmail.com>> > wrote: > >> Thank you for your clarification, Chris, >> >> I have spent some time to review KIP-894 and I think it's automatic way is >> better and bring no side effect, and I will also adopt this way here. >> As you mentioned, the changes in semantics is minor, the most important >> reason for this change is fixing bug brought by sensitive configs. >> >> >>> We >>> don't appear to support appending/subtracting from list properties via >> the >>> CLI for any other entity type right now, >> You are right about this, I tried and found that we can’t subtract or >> append configs, I will change the KIP to "making way for >> appending/subtracting list properties" >> >> -- >> Best, >> Ziming >> >>> On Jan 6, 2024, at 01:34, Chris Egerton <chr...@aiven.io.INVALID> wrote: >>> >>> Hi all, >>> >>> Can we clarify any changes in the user-facing semantics for the CLI tool >>> that would come about as a result of this KIP? I think the debate over >> the >>> necessity of an opt-in flag, or waiting for 4.0.0, ultimately boils down >> to >>> this. >>> >>> My understanding is that the only changes in semantics are fairly minor >>> (semantic versioning pun intended): >>> >>> - Existing sensitive broker properties no longer have to be explicitly >>> specified on the command line if they're not being changed >>> - A small race condition is fixed where the broker config is updated by a >>> separate operation in between when the CLI reads the existing broker >> config >>> and writes the new broker config >>> - Usage of a new broker API that has been supported since version 2.3.0, >>> but which does not require any new ACLs and does not act any differently >>> apart from the two small changes noted above >>> >>> If this is correct, then I'm inclined to agree with Ismael's suggestion >> of >>> starting with incrementalAlterConfigs, and falling back on alterConfigs >> if >>> the former is not supported by the broker, and do not believe it's >>> necessary to wait for 4.0.0 or provide opt-in or opt-out flags to release >>> this change. This would also be similar to changes we made to >> MirrorMaker 2 >>> in KIP-894 [1], where the default behavior for syncing topic configs is >> now >>> to start with incrementalAlterConfigs and fall back on alterConfigs if >> it's >>> not supported. >>> >>> If there are other, more significant changes to the user-facing semantics >>> for the CLI, then these should be called out here and in the KIP, and we >>> might consider a more cautious approach. >>> >>> [1] - >>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-894%3A+Use+incrementalAlterConfigs+API+for+syncing+topic+configurations >>> >>> >>> Also, regarding this part of the KIP: >>> >>>> incrementalAlterConfigs is more convenient especially for updating >>> configs of list data type, such as >> "leader.replication.throttled.replicas" >>> >>> While this is true for the Java admin client and the corresponding broker >>> APIs, it doesn't appear to be relevant to the kafka-configs.sh CLI tool. >> We >>> don't appear to support appending/subtracting from list properties via >> the >>> CLI for any other entity type right now, and there's nothing in the KIP >>> that leads me to believe we'd be adding it for broker configs. >>> >>> Cheers, >>> >>> Chris >>> >>> On Thu, Jan 4, 2024 at 10:12 PM ziming deng <dengziming1...@gmail.com >> <mailto:dengziming1...@gmail.com>> >>> wrote: >>> >>>> Hi Ismael, >>>> I added this automatically approach to “Rejected alternatives” >> concerning >>>> that we need to unify the semantics between alterConfigs and >>>> incrementalAlterConfigs, so I choose to give this privilege to users. >>>> >>>> After reviewing these code and doing some tests I found that they >>>> following the similar approach, I think the simplest way is to let the >>>> client choose the best method heuristically. >>>> >>>> Thank you for pointing out this, I will change the KIP later. >>>> >>>> Best, >>>> Ziming >>>> >>>>> On Jan 4, 2024, at 17:28, Ismael Juma <m...@ismaeljuma.com> wrote: >>>>> >>>>> Hi Ziming, >>>>> >>>>> Why is the flag required at all? Can we use incremental and fallback >>>> automatically if it's not supported by the broker? At this point, the >> vast >>>> majority of clusters should support it. >>>>> >>>>> Ismael >>>>> >>>>> On Mon, Dec 18, 2023 at 7:58 PM ziming deng <dengziming1...@gmail.com >>>> <mailto:dengziming1...@gmail.com>> wrote: >>>>>> >>>>>> Hello, I want to start a discussion on KIP-1011, to make the broker >>>> config change path unified with that of user/topic/client-metrics and >> avoid >>>> some bugs. >>>>>> >>>>>> Here is the link: >>>>>> >>>>>> KIP-1011: Use incrementalAlterConfigs when updating broker configs by >>>> kafka-configs.sh - Apache Kafka - Apache Software Foundation >>>>>> cwiki.apache.org <http://cwiki.apache.org/> <http://cwiki.apache.org/> >>>>>> >>>>>> < >>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1011%3A+Use+incrementalAlterConfigs+when+updating+broker+configs+by+kafka-configs.sh >>> KIP-1011: >>>> Use incrementalAlterConfigs when updating broker configs by >>>> kafka-configs.sh - Apache Kafka - Apache Software Foundation < >>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1011%3A+Use+incrementalAlterConfigs+when+updating+broker+configs+by+kafka-configs.sh >>>>> >>>>>> cwiki.apache.org <http://cwiki.apache.org/> <http://cwiki.apache.org/> < >>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1011%3A+Use+incrementalAlterConfigs+when+updating+broker+configs+by+kafka-configs.sh >>> >>>> < >>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1011%3A+Use+incrementalAlterConfigs+when+updating+broker+configs+by+kafka-configs.sh >>>>> >>>>>> >>>>>> Best, >>>>>> Ziming.
