KIP is updated now: https://cwiki.apache.org/confluence/display/KAFKA/KIP-935%3A+Extend+AlterConfigPolicy+with+existing+configurations
Looking forward to your feedback, Many thanks, Jorge. On Tue, 25 Jul 2023 at 16:59, Jorge Esteban Quilcate Otoya < quilcate.jo...@gmail.com> wrote: > Hi Colin, sorry for the belated follow up. > > If I understand correctly, on your latest reply proposed to have a new > API. From the proposed alternatives, I lean towards the first alternative > proposed with 2 config maps, old (before-alter) and new (after-alter). > Deleting a config is effectively returning to the default value, then users > can use the old value and compare against default if new is null. > > This would require a bit broader changes, starting with a new config. I > will work on the KIP updates considering: `AlterConfigV2Policy` interface, > and config `alter.config.policy.v2.class.name`. Let me know if there's > any issues with this; otherwise I will update the mail thread once the KIP > is updated. > > Many thanks, > Jorge. > > On Tue, 20 Jun 2023 at 11:56, Jorge Esteban Quilcate Otoya < > quilcate.jo...@gmail.com> wrote: > >> Thanks Colin! You're right. I started this KIP only thinking on the >> latest incremental API, and haven't thought much on the legacy one. >> >> After taking a another look at both APIs, I can see some inconsistencies >> on how the policies are applied in each case. I have added a section >> "Current workflow" [1] to the current proposal to summarize how alter >> config works in both cases (legacy and incremental) and for both back-ends >> (ZK, KRaft). >> >> In summary: >> - On Legacy Alter Config, the set of changes proposed is the same as the >> new config with the difference that null values are removed from the new >> config. >> - On Incremental Alter Config, the set of changes proposed is not the >> same as the new config. It only contains explicit changes to the config >> - Implicit deletes are a set of configs inferred on legacy alter config >> when no value is provided but it exists on the current config >> - Even though alter config policy receives the "requested" >> configurations, these have 2 different meanings depending on the API used >> to update configs. >> - When validating policies on Legacy Alter Config, it means: requested >> changes that is equal to new config state including explicit deletes >> - When validating policies on Incremental Alter Config, it means: only >> requested changes including explicit deletes but without any other config >> from current or new status >> - Plugin implementations *do not know which one are they actually >> dealing with*, and as incremental (new) API becomes broadly adopted, then >> current status configs not included in the request are not considered. >> >> The problem is a bit larger than the one framed on the motivation. It's >> not only that we don't have the current configs to compare with; but >> depending on the API used to alter configs we may have them or not. >> >> Is this assessment correct? >> If it is, then we may discuss approaching this issue as a bug instead. We >> could consider aligning the semantics of the configs passed to the policy. >> At the moment the "requested configs" are passed to policies when either >> API is called, but both have _different meaning depending on which API is >> used_. We could instead align the meaning, and pass the "new configs, >> including explicit deletes" as we do on legacy when doing incremental >> updates as well. >> >> Looking forward to your feedback and many thanks again! >> Jorge. >> >> [1] >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-935%3A+Extend+AlterConfigPolicy+with+existing+configurations#KIP935:ExtendAlterConfigPolicywithexistingconfigurations-Currentworkflow >> >> On Thu, 15 Jun 2023 at 22:07, Colin McCabe <cmcc...@apache.org> wrote: >> >>> Hi Jorge, >>> >>> I appreciate you trying to solve the issue. However, from the >>> perspective of someone using the plugin API, it's quite messy: what is the >>> difference between "proposed" and "resulting"? They sound the same. >>> >>> I think there are two APIs that make sense: >>> >>> 1. A (prev, next) based one where you just get the previous set of >>> configs, and the new one, and can draw your own conclusions >>> >>> 2. A (prev, changed, removed) one where you get the previous set of >>> configs, plus the changes (additions or modifications), and deletions. >>> >>> 3. Same as 2 but you have a "changed" map whose values are Optionals, >>> and express deletions as Optional.empty >>> >>> The old API should just stay the same, bugs and all, for compatibility >>> reasons. But for the new API we should choose one of the above, I think. >>> I'm not completely sure which... >>> >>> best, >>> Colin >>> >>> On Mon, Jun 12, 2023, at 07:08, Jorge Esteban Quilcate Otoya wrote: >>> > Thanks Colin! You're right. I have added some notes about this to the >>> KIP, >>> > and clarify how this KIP is related to legacy and incremental alter >>> config >>> > APIs. >>> > >>> > Let me know if there's any gaps on the current proposal. >>> > >>> > Many thanks, >>> > Jorge. >>> > >>> > On Mon, 12 Jun 2023 at 11:04, Colin McCabe <co...@cmccabe.xyz> wrote: >>> > >>> >> See KAFKA-14195. Some deletions are not handled correctly. And this >>> cannot >>> >> be fixed without a kip because of backwards compatibility. >>> >> >>> >> Colin >>> >> >>> >> On Wed, Jun 7, 2023, at 17:07, Jorge Esteban Quilcate Otoya wrote: >>> >> > Thank Colin. >>> >> > >>> >> > I've took a closer look on how configs are passed to the policy when >>> >> > delete >>> >> > configs are requested, and either null (KRaft) or empty values >>> >> > (ZkAdminManager) are passed: >>> >> > - ZkAdminManager passes empty values: >>> >> > - Config Entries definition: >>> >> > >>> >> >>> https://github.com/apache/kafka/blob/513e1c641d63c5e15144f9fcdafa1b56c5e5ba09/core/src/main/scala/kafka/server/ZkAdminManager.scala#L503 >>> >> > - and passed to policy without changes: >>> >> > >>> >> >>> https://github.com/apache/kafka/blob/513e1c641d63c5e15144f9fcdafa1b56c5e5ba09/core/src/main/scala/kafka/server/ZkAdminManager.scala#L495 >>> >> > - Similar with ConfigurationControlManager (KRaft) passes null >>> values: >>> >> > - Config entries added regardless of value==null: >>> >> > >>> >> >>> https://github.com/apache/kafka/blob/513e1c641d63c5e15144f9fcdafa1b56c5e5ba09/metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java#L281 >>> >> > - And passed to policy: >>> >> > >>> >> >>> https://github.com/apache/kafka/blob/513e1c641d63c5e15144f9fcdafa1b56c5e5ba09/metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java#L295 >>> >> > >>> >> > So, instead of passing (requested config + requested config to >>> delete + >>> >> > existing configs), the new metadata type is including (requested >>> >> > configs--which already include deleted configs-- + _resulting_ >>> configs) >>> >> so >>> >> > users could evaluate the whole (resulting) config map similar to >>> >> > CreateTopicPolicy; and check only requested configs if needed (as >>> with >>> >> > current metadata). >>> >> > >>> >> > I've also added a rejected alternative to consider the scenario of >>> >> > piggybacking on the existing map and just including the resulting >>> config >>> >> > instead, though this would break compatibility with existing >>> >> > implementations. >>> >> > >>> >> > Thanks, >>> >> > Jorge. >>> >> > >>> >> > >>> >> > On Wed, 7 Jun 2023 at 08:38, Colin McCabe <cmcc...@apache.org> >>> wrote: >>> >> > >>> >> >> On Tue, Jun 6, 2023, at 06:57, Jorge Esteban Quilcate Otoya wrote: >>> >> >> > Thanks Colin. >>> >> >> > >>> >> >> >> I would suggest renaming the "configs" parameter to >>> >> "proposedConfigs," >>> >> >> in >>> >> >> > both the new and old RequestMetadata constructors, to make things >>> >> >> clearer. >>> >> >> > This would be a binary and API-compatible change in Java. >>> >> >> > >>> >> >> > Sure, fully agree. KIP is updated with this suggestion. >>> >> >> > >>> >> >> >> We should also clarify that if configurations are being >>> proposed for >>> >> >> > deletion, they won't appear in proposedConfigs. >>> >> >> > >>> >> >> > Great catch. Wasn't aware of this, but I think it's valuable for >>> the >>> >> API >>> >> >> to >>> >> >> > also include the list of configurations to be deleted. >>> >> >> > For this, I have extended the `RequestMetadata` type with a list >>> of >>> >> >> > proposed configs to delete: >>> >> >> > >>> >> >> >>> >> >> Hi Jorge, >>> >> >> >>> >> >> Thanks for the reply. >>> >> >> >>> >> >> Rather than having a separate list of proposedConfigsToDelete, it >>> seems >>> >> >> like we could have an accessor function that calculates this when >>> >> needed. >>> >> >> After all, it's completely determined by existingConfigs and >>> >> >> proposedConfigs. And some plugins will want the list and some >>> won't (or >>> >> >> will want to do a slightly different analysis) >>> >> >> >>> >> >> regards, >>> >> >> Colin >>> >> >> >>> >> >> >>> >> >> > ``` >>> >> >> > class RequestMetadata { >>> >> >> > >>> >> >> > private final ConfigResource resource; >>> >> >> > private final Map<String, String> proposedConfigs; >>> >> >> > private final List<String> proposedConfigsToDelete; >>> >> >> > private final Map<String, String> existingConfigs; >>> >> >> > ``` >>> >> >> > >>> >> >> > Looking forward to your feedback. >>> >> >> > >>> >> >> > Cheers, >>> >> >> > Jorge. >>> >> >> > >>> >> >> > On Fri, 2 Jun 2023 at 22:42, Colin McCabe <cmcc...@apache.org> >>> wrote: >>> >> >> > >>> >> >> >> Hi Jorge, >>> >> >> >> >>> >> >> >> This is a good KIP which addresses a real gap we have today. >>> >> >> >> >>> >> >> >> I would suggest renaming the "configs" parameter to >>> >> "proposedConfigs," >>> >> >> in >>> >> >> >> both the new and old RequestMetadata constructors, to make >>> things >>> >> >> clearer. >>> >> >> >> This would be a binary and API-compatible change in Java. We >>> should >>> >> also >>> >> >> >> clarify that if configurations are being proposed for deletion, >>> they >>> >> >> won't >>> >> >> >> appear in proposedConfigs. >>> >> >> >> >>> >> >> >> best, >>> >> >> >> Colin >>> >> >> >> >>> >> >> >> >>> >> >> >> On Tue, May 23, 2023, at 03:03, Christo Lolov wrote: >>> >> >> >> > Hello! >>> >> >> >> > >>> >> >> >> > This proposal will address problems with configuration >>> dependencies >>> >> >> >> which I >>> >> >> >> > run into very frequently, so I am fully supporting the >>> development >>> >> of >>> >> >> >> this >>> >> >> >> > feature! >>> >> >> >> > >>> >> >> >> > Best, >>> >> >> >> > Christo >>> >> >> >> > >>> >> >> >> > On Mon, 22 May 2023 at 17:18, Jorge Esteban Quilcate Otoya < >>> >> >> >> > quilcate.jo...@gmail.com> wrote: >>> >> >> >> > >>> >> >> >> >> Hi everyone, >>> >> >> >> >> >>> >> >> >> >> I'd like to start a discussion for KIP-935 < >>> >> >> >> >> >>> >> >> >> >> >>> >> >> >> >>> >> >> >>> >> >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-935%3A+Extend+AlterConfigPolicy+with+existing+configurations >>> >> >> >> >> > >>> >> >> >> >> which proposes extend AlterConfigPolicy with existing >>> >> configuration >>> >> >> to >>> >> >> >> >> enable more complex policies. >>> >> >> >> >> >>> >> >> >> >> There have been related KIPs in the past that haven't been >>> >> accepted >>> >> >> and >>> >> >> >> >> seem retired/abandoned as outlined in the motivation. >>> >> >> >> >> The scope of this KIP intends to be more specific to get >>> most of >>> >> the >>> >> >> >> >> benefits from previous discussions; and if previous KIPs are >>> >> >> >> resurrected, >>> >> >> >> >> should still be possible to do it if this one is adopted. >>> >> >> >> >> >>> >> >> >> >> Looking forward to your feedback! >>> >> >> >> >> >>> >> >> >> >> Thanks, >>> >> >> >> >> Jorge. >>> >> >> >> >> >>> >> >> >> >>> >> >> >>> >> >>> >>
