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