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