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