Thanks Ziming, LGTM! On Mon, Jan 15, 2024 at 12:00 AM ziming deng <dengziming1...@gmail.com> wrote:
> 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. > >
