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