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