You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jorge Esteban Quilcate Otoya <qu...@gmail.com> on 2023/05/22 16:18:21 UTC
[DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
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.
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Jorge Esteban Quilcate Otoya <qu...@gmail.com>.
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.jorge@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.jorge@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 <cm...@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 <cm...@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 <cm...@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.jorge@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.
>>> >> >> >> >>
>>> >> >> >>
>>> >> >>
>>> >>
>>>
>>
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Jorge Esteban Quilcate Otoya <qu...@gmail.com>.
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.jorge@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 <cm...@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 <cm...@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 <cm...@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.jorge@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.
>> >> >> >> >>
>> >> >> >>
>> >> >>
>> >>
>>
>
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Jorge Esteban Quilcate Otoya <qu...@gmail.com>.
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 <cm...@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 <cm...@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 <cm...@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.jorge@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.
> >> >> >> >>
> >> >> >>
> >> >>
> >>
>
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Colin McCabe <cm...@apache.org>.
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 <cm...@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 <cm...@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.jorge@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.
>> >> >> >>
>> >> >>
>> >>
>>
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Jorge Esteban Quilcate Otoya <qu...@gmail.com>.
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 <cm...@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 <cm...@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.jorge@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.
> >> >> >>
> >> >>
> >>
>
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Colin McCabe <co...@cmccabe.xyz>.
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 <cm...@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 <cm...@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.jorge@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.
>> >> >>
>> >>
>>
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Jorge Esteban Quilcate Otoya <qu...@gmail.com>.
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 <cm...@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 <cm...@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.jorge@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.
> >> >>
> >>
>
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Colin McCabe <cm...@apache.org>.
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 <cm...@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.jorge@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.
>> >>
>>
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Jorge Esteban Quilcate Otoya <qu...@gmail.com>.
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:
```
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 <cm...@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.jorge@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.
> >>
>
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Colin McCabe <cm...@apache.org>.
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.jorge@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.
>>
Re: [DISCUSS] KIP-935: Extend AlterConfigPolicy with existing configurations
Posted by Christo Lolov <ch...@gmail.com>.
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.jorge@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.
>