[DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Stanislav Kozlovski <st...@confluent.io.INVALID>]

Hey all,

I'd like to start a discussion on a proposal to help API users from
inadvertently increasing the replication factor of a topic through
the alter partition reassignments API. The KIP describes two fairly
easy-to-hit race conditions in which this can happen.

The KIP itself is pretty simple, yet has a couple of alternatives that can
help solve the same problem. I would appreciate thoughts from the community
on how you think we should proceed, and whether the proposal makes sense in
the first place.

Thanks!

KIP:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
JIRA: https://issues.apache.org/jira/browse/KAFKA-14121

-- 
Best,
Stanislav

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Stanislav Kozlovski <st...@confluent.io.INVALID>]

Thanks Ismael,

I added an extra paragraph in the motivation. We have certainly hit this
within our internal Confluent reassignment software and from a quick skim
in the popular Cruise Control repository, I notice that similar problems
have been hit there too. Hopefully the examples in the KIP are sufficient
to make the case

On Wed, Sep 7, 2022 at 11:21 PM Ismael Juma <is...@juma.me.uk> wrote:

> Thanks for the details, Colin. I understand how this can happen. But this
> API has been out for a long time. Are we saying that we have seen Cruise
> Control cause this kind of problem? If so, it would be good to mention it
> in the KIP as evidence that the current approach is brittle.
>
> Ismael
>
> On Wed, Sep 7, 2022 at 2:15 PM Colin McCabe <cm...@apache.org> wrote:
>
> > Hi Ismael,
> >
> > I think this issue comes up when people write software that automatically
> > creates partition reassignments to balance the cluster. Cruise Control is
> > one example; Confluent also has some software that does this. If there is
> > already a reassignment that is going on for some partition and the
> software
> > tries to create a new reassignment for that partition, the software may
> > inadvertently change the replication factor.
> >
> > In general, I think some people find it surprising that reassignment can
> > change the replication factor of a partition. When we outlined the
> > reassignment API in KIP-455 we maintained the ability to do this, since
> the
> > old ZK-based API had always been able to do it. But this was a bit
> > controversial. Maybe it would have been more intuitive to preserve
> > replication factor by default unless the user explicitly stated that they
> > wanted to change it. So in a sense, you could view this as a fix for
> > KIP-455 :) (in my opinion, at least)
> >
> > best,
> > Colin
> >
> >
> > On Wed, Sep 7, 2022, at 07:07, Ismael Juma wrote:
> > > Thanks for the KIP. Can we explain a bit more why this is an important
> > use
> > > case to address? For example, do we have concrete examples of people
> > > running into this? The way the KIP is written, it sounds like a
> potential
> > > problem but no information is given on whether it's a real problem in
> > > practice.
> > >
> > > Ismael
> > >
> > > On Thu, Jul 28, 2022 at 2:00 AM Stanislav Kozlovski
> > > <st...@confluent.io.invalid> wrote:
> > >
> > >> Hey all,
> > >>
> > >> I'd like to start a discussion on a proposal to help API users from
> > >> inadvertently increasing the replication factor of a topic through
> > >> the alter partition reassignments API. The KIP describes two fairly
> > >> easy-to-hit race conditions in which this can happen.
> > >>
> > >> The KIP itself is pretty simple, yet has a couple of alternatives that
> > can
> > >> help solve the same problem. I would appreciate thoughts from the
> > community
> > >> on how you think we should proceed, and whether the proposal makes
> > sense in
> > >> the first place.
> > >>
> > >> Thanks!
> > >>
> > >> KIP:
> > >>
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > >> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> > >>
> > >> --
> > >> Best,
> > >> Stanislav
> > >>
> >
>


-- 
Best,
Stanislav

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Ismael Juma <is...@juma.me.uk>]

Thanks for the details, Colin. I understand how this can happen. But this
API has been out for a long time. Are we saying that we have seen Cruise
Control cause this kind of problem? If so, it would be good to mention it
in the KIP as evidence that the current approach is brittle.

Ismael

On Wed, Sep 7, 2022 at 2:15 PM Colin McCabe <cm...@apache.org> wrote:

> Hi Ismael,
>
> I think this issue comes up when people write software that automatically
> creates partition reassignments to balance the cluster. Cruise Control is
> one example; Confluent also has some software that does this. If there is
> already a reassignment that is going on for some partition and the software
> tries to create a new reassignment for that partition, the software may
> inadvertently change the replication factor.
>
> In general, I think some people find it surprising that reassignment can
> change the replication factor of a partition. When we outlined the
> reassignment API in KIP-455 we maintained the ability to do this, since the
> old ZK-based API had always been able to do it. But this was a bit
> controversial. Maybe it would have been more intuitive to preserve
> replication factor by default unless the user explicitly stated that they
> wanted to change it. So in a sense, you could view this as a fix for
> KIP-455 :) (in my opinion, at least)
>
> best,
> Colin
>
>
> On Wed, Sep 7, 2022, at 07:07, Ismael Juma wrote:
> > Thanks for the KIP. Can we explain a bit more why this is an important
> use
> > case to address? For example, do we have concrete examples of people
> > running into this? The way the KIP is written, it sounds like a potential
> > problem but no information is given on whether it's a real problem in
> > practice.
> >
> > Ismael
> >
> > On Thu, Jul 28, 2022 at 2:00 AM Stanislav Kozlovski
> > <st...@confluent.io.invalid> wrote:
> >
> >> Hey all,
> >>
> >> I'd like to start a discussion on a proposal to help API users from
> >> inadvertently increasing the replication factor of a topic through
> >> the alter partition reassignments API. The KIP describes two fairly
> >> easy-to-hit race conditions in which this can happen.
> >>
> >> The KIP itself is pretty simple, yet has a couple of alternatives that
> can
> >> help solve the same problem. I would appreciate thoughts from the
> community
> >> on how you think we should proceed, and whether the proposal makes
> sense in
> >> the first place.
> >>
> >> Thanks!
> >>
> >> KIP:
> >>
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> >> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> >>
> >> --
> >> Best,
> >> Stanislav
> >>
>

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Colin McCabe <cm...@apache.org>]

Hi Ismael,

I think this issue comes up when people write software that automatically creates partition reassignments to balance the cluster. Cruise Control is one example; Confluent also has some software that does this. If there is already a reassignment that is going on for some partition and the software tries to create a new reassignment for that partition, the software may inadvertently change the replication factor.

In general, I think some people find it surprising that reassignment can change the replication factor of a partition. When we outlined the reassignment API in KIP-455 we maintained the ability to do this, since the old ZK-based API had always been able to do it. But this was a bit controversial. Maybe it would have been more intuitive to preserve replication factor by default unless the user explicitly stated that they wanted to change it. So in a sense, you could view this as a fix for KIP-455 :) (in my opinion, at least)

best,
Colin


On Wed, Sep 7, 2022, at 07:07, Ismael Juma wrote:
> Thanks for the KIP. Can we explain a bit more why this is an important use
> case to address? For example, do we have concrete examples of people
> running into this? The way the KIP is written, it sounds like a potential
> problem but no information is given on whether it's a real problem in
> practice.
>
> Ismael
>
> On Thu, Jul 28, 2022 at 2:00 AM Stanislav Kozlovski
> <st...@confluent.io.invalid> wrote:
>
>> Hey all,
>>
>> I'd like to start a discussion on a proposal to help API users from
>> inadvertently increasing the replication factor of a topic through
>> the alter partition reassignments API. The KIP describes two fairly
>> easy-to-hit race conditions in which this can happen.
>>
>> The KIP itself is pretty simple, yet has a couple of alternatives that can
>> help solve the same problem. I would appreciate thoughts from the community
>> on how you think we should proceed, and whether the proposal makes sense in
>> the first place.
>>
>> Thanks!
>>
>> KIP:
>>
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
>> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
>>
>> --
>> Best,
>> Stanislav
>>

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Ismael Juma <is...@juma.me.uk>]

Thanks for the KIP. Can we explain a bit more why this is an important use
case to address? For example, do we have concrete examples of people
running into this? The way the KIP is written, it sounds like a potential
problem but no information is given on whether it's a real problem in
practice.

Ismael

On Thu, Jul 28, 2022 at 2:00 AM Stanislav Kozlovski
<st...@confluent.io.invalid> wrote:

> Hey all,
>
> I'd like to start a discussion on a proposal to help API users from
> inadvertently increasing the replication factor of a topic through
> the alter partition reassignments API. The KIP describes two fairly
> easy-to-hit race conditions in which this can happen.
>
> The KIP itself is pretty simple, yet has a couple of alternatives that can
> help solve the same problem. I would appreciate thoughts from the community
> on how you think we should proceed, and whether the proposal makes sense in
> the first place.
>
> Thanks!
>
> KIP:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
>
> --
> Best,
> Stanislav
>

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Kuan Po Tseng <br...@apache.org>]

Hi everyone,

Just a quick update—I’ve made some adjustments to the KIP:
- Changed allowReplicationFactorChange from Boolean to boolean.
- Adopted the fluent pattern in AlterPartitionReassignmentsOptions.

Also, the PR has been merged into trunk, targeting version 4.1.0. 
Thanks again for your support!

Best,
Kuan-Po Tseng

On 2025/02/06 18:42:29 Chia-Ping Tsai wrote:
> hi Stan
> 
> thanks for sharing the details!
> 
> to Kuan-Po
> 
> Could you please tweak the KIP-860 according to Stan's response :)
> 
> Best,
> Chia-Ping
> 
> Stanislav Kozlovski <st...@apache.org> 於 2025年2月7日 週五 上午2:32寫道：
> 
> > Hey Kuan-Po!
> >
> > First of all - thanks so much for volunteering to take the implementation
> > of this KIP!
> >
> > When I initially proposed it, I had planned with my team in Confluent to
> > have somebody else implement it as a chance to get more experience with the
> > open-source project. Sadly, we never got to it.
> >
> > To answer your question:
> >
> > The idea of this KIP is to prevent the replication factor from changing,
> > so it's ultimately only the size that should get checked. As mentioned - if
> > the partition is NOT reassigning, then we check against
> > `targetReplicaSet.size != currentReplicaSet.size`. If the partition IS
> > reasigning, then we check `currentReassignmentTargetReplicaSet.size !=
> > newTargetReplicaSet.size`
> >
> > Best,
> > Stan
> >
> > On 2025/02/05 14:46:08 Kuan Po Tseng wrote:
> > > Hi all,
> > >
> > > After discussing with Stanislav Kozlovski, I plan to continue
> > implementing this KIP.
> > > However, I have a question regarding the Validation section.
> > >
> > > It says, "If the targetReplicaSet of the reassignment differs from the
> > current replica set
> > > of the partition, an error is thrown."
> > >
> > > I’m wondering if the word “differs” here refers to:
> > > (1) Only the targetReplicaSet size differing from the current replica
> > set, or
> > > (2) The targetReplicaSet as a whole differing from the current replica
> > set.
> > >
> > > I assume it’s (2) since the KIP aims to avoid a situation where, during
> > alterPartitionReassignments, the replication factor (RF) could end up
> > > larger than the original RF. However, I would like to confirm if
> > > my understanding is correct.
> > >
> > > Thank you!
> > >
> > > --
> > > Best,
> > > Kuan-Po
> > >
> > > On 2022/07/28 08:59:18 Stanislav Kozlovski wrote:
> > > > Hey all,
> > > >
> > > > I'd like to start a discussion on a proposal to help API users from
> > > > inadvertently increasing the replication factor of a topic through
> > > > the alter partition reassignments API. The KIP describes two fairly
> > > > easy-to-hit race conditions in which this can happen.
> > > >
> > > > The KIP itself is pretty simple, yet has a couple of alternatives that
> > can
> > > > help solve the same problem. I would appreciate thoughts from the
> > community
> > > > on how you think we should proceed, and whether the proposal makes
> > sense in
> > > > the first place.
> > > >
> > > > Thanks!
> > > >
> > > > KIP:
> > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > > > JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> > > >
> > > > --
> > > > Best,
> > > > Stanislav
> > > >
> > >
> >
> 

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Kuan Po Tseng <br...@apache.org>]

Hi everyone,

Thanks to Stanislav for answering the question!
In the beginning I thought
I’ve also updated the KIP based on the discussion above.

Best,
Kuan-Po

On 2025/02/06 18:42:29 Chia-Ping Tsai wrote:
> hi Stan
> 
> thanks for sharing the details!
> 
> to Kuan-Po
> 
> Could you please tweak the KIP-860 according to Stan's response :)
> 
> Best,
> Chia-Ping
> 
> Stanislav Kozlovski <st...@apache.org> 於 2025年2月7日 週五 上午2:32寫道：
> 
> > Hey Kuan-Po!
> >
> > First of all - thanks so much for volunteering to take the implementation
> > of this KIP!
> >
> > When I initially proposed it, I had planned with my team in Confluent to
> > have somebody else implement it as a chance to get more experience with the
> > open-source project. Sadly, we never got to it.
> >
> > To answer your question:
> >
> > The idea of this KIP is to prevent the replication factor from changing,
> > so it's ultimately only the size that should get checked. As mentioned - if
> > the partition is NOT reassigning, then we check against
> > `targetReplicaSet.size != currentReplicaSet.size`. If the partition IS
> > reasigning, then we check `currentReassignmentTargetReplicaSet.size !=
> > newTargetReplicaSet.size`
> >
> > Best,
> > Stan
> >
> > On 2025/02/05 14:46:08 Kuan Po Tseng wrote:
> > > Hi all,
> > >
> > > After discussing with Stanislav Kozlovski, I plan to continue
> > implementing this KIP.
> > > However, I have a question regarding the Validation section.
> > >
> > > It says, "If the targetReplicaSet of the reassignment differs from the
> > current replica set
> > > of the partition, an error is thrown."
> > >
> > > I’m wondering if the word “differs” here refers to:
> > > (1) Only the targetReplicaSet size differing from the current replica
> > set, or
> > > (2) The targetReplicaSet as a whole differing from the current replica
> > set.
> > >
> > > I assume it’s (2) since the KIP aims to avoid a situation where, during
> > alterPartitionReassignments, the replication factor (RF) could end up
> > > larger than the original RF. However, I would like to confirm if
> > > my understanding is correct.
> > >
> > > Thank you!
> > >
> > > --
> > > Best,
> > > Kuan-Po
> > >
> > > On 2022/07/28 08:59:18 Stanislav Kozlovski wrote:
> > > > Hey all,
> > > >
> > > > I'd like to start a discussion on a proposal to help API users from
> > > > inadvertently increasing the replication factor of a topic through
> > > > the alter partition reassignments API. The KIP describes two fairly
> > > > easy-to-hit race conditions in which this can happen.
> > > >
> > > > The KIP itself is pretty simple, yet has a couple of alternatives that
> > can
> > > > help solve the same problem. I would appreciate thoughts from the
> > community
> > > > on how you think we should proceed, and whether the proposal makes
> > sense in
> > > > the first place.
> > > >
> > > > Thanks!
> > > >
> > > > KIP:
> > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > > > JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> > > >
> > > > --
> > > > Best,
> > > > Stanislav
> > > >
> > >
> >
> 

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Chia-Ping Tsai <ch...@gmail.com>]

hi Stan

thanks for sharing the details!

to Kuan-Po

Could you please tweak the KIP-860 according to Stan's response :)

Best,
Chia-Ping

Stanislav Kozlovski <st...@apache.org> 於 2025年2月7日 週五 上午2:32寫道：

> Hey Kuan-Po!
>
> First of all - thanks so much for volunteering to take the implementation
> of this KIP!
>
> When I initially proposed it, I had planned with my team in Confluent to
> have somebody else implement it as a chance to get more experience with the
> open-source project. Sadly, we never got to it.
>
> To answer your question:
>
> The idea of this KIP is to prevent the replication factor from changing,
> so it's ultimately only the size that should get checked. As mentioned - if
> the partition is NOT reassigning, then we check against
> `targetReplicaSet.size != currentReplicaSet.size`. If the partition IS
> reasigning, then we check `currentReassignmentTargetReplicaSet.size !=
> newTargetReplicaSet.size`
>
> Best,
> Stan
>
> On 2025/02/05 14:46:08 Kuan Po Tseng wrote:
> > Hi all,
> >
> > After discussing with Stanislav Kozlovski, I plan to continue
> implementing this KIP.
> > However, I have a question regarding the Validation section.
> >
> > It says, "If the targetReplicaSet of the reassignment differs from the
> current replica set
> > of the partition, an error is thrown."
> >
> > I’m wondering if the word “differs” here refers to:
> > (1) Only the targetReplicaSet size differing from the current replica
> set, or
> > (2) The targetReplicaSet as a whole differing from the current replica
> set.
> >
> > I assume it’s (2) since the KIP aims to avoid a situation where, during
> alterPartitionReassignments, the replication factor (RF) could end up
> > larger than the original RF. However, I would like to confirm if
> > my understanding is correct.
> >
> > Thank you!
> >
> > --
> > Best,
> > Kuan-Po
> >
> > On 2022/07/28 08:59:18 Stanislav Kozlovski wrote:
> > > Hey all,
> > >
> > > I'd like to start a discussion on a proposal to help API users from
> > > inadvertently increasing the replication factor of a topic through
> > > the alter partition reassignments API. The KIP describes two fairly
> > > easy-to-hit race conditions in which this can happen.
> > >
> > > The KIP itself is pretty simple, yet has a couple of alternatives that
> can
> > > help solve the same problem. I would appreciate thoughts from the
> community
> > > on how you think we should proceed, and whether the proposal makes
> sense in
> > > the first place.
> > >
> > > Thanks!
> > >
> > > KIP:
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > > JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> > >
> > > --
> > > Best,
> > > Stanislav
> > >
> >
>

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Stanislav Kozlovski <st...@apache.org>]

Hey Kuan-Po!

First of all - thanks so much for volunteering to take the implementation of this KIP!

When I initially proposed it, I had planned with my team in Confluent to have somebody else implement it as a chance to get more experience with the open-source project. Sadly, we never got to it.

To answer your question:

The idea of this KIP is to prevent the replication factor from changing, so it's ultimately only the size that should get checked. As mentioned - if the partition is NOT reassigning, then we check against `targetReplicaSet.size != currentReplicaSet.size`. If the partition IS reasigning, then we check `currentReassignmentTargetReplicaSet.size != newTargetReplicaSet.size`

Best,
Stan

On 2025/02/05 14:46:08 Kuan Po Tseng wrote:
> Hi all,
> 
> After discussing with Stanislav Kozlovski, I plan to continue implementing this KIP.
> However, I have a question regarding the Validation section.
> 
> It says, "If the targetReplicaSet of the reassignment differs from the current replica set
> of the partition, an error is thrown."
> 
> I’m wondering if the word “differs” here refers to:
> (1) Only the targetReplicaSet size differing from the current replica set, or
> (2) The targetReplicaSet as a whole differing from the current replica set.
> 
> I assume it’s (2) since the KIP aims to avoid a situation where, during alterPartitionReassignments, the replication factor (RF) could end up
> larger than the original RF. However, I would like to confirm if 
> my understanding is correct.
> 
> Thank you!
> 
> --
> Best,
> Kuan-Po
> 
> On 2022/07/28 08:59:18 Stanislav Kozlovski wrote:
> > Hey all,
> > 
> > I'd like to start a discussion on a proposal to help API users from
> > inadvertently increasing the replication factor of a topic through
> > the alter partition reassignments API. The KIP describes two fairly
> > easy-to-hit race conditions in which this can happen.
> > 
> > The KIP itself is pretty simple, yet has a couple of alternatives that can
> > help solve the same problem. I would appreciate thoughts from the community
> > on how you think we should proceed, and whether the proposal makes sense in
> > the first place.
> > 
> > Thanks!
> > 
> > KIP:
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> > 
> > -- 
> > Best,
> > Stanislav
> > 
> 

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Kuan Po Tseng <br...@apache.org>]

Hi all,

After discussing with Stanislav Kozlovski, I plan to continue implementing this KIP.
However, I have a question regarding the Validation section.

It says, "If the targetReplicaSet of the reassignment differs from the current replica set
of the partition, an error is thrown."

I’m wondering if the word “differs” here refers to:
(1) Only the targetReplicaSet size differing from the current replica set, or
(2) The targetReplicaSet as a whole differing from the current replica set.

I assume it’s (2) since the KIP aims to avoid a situation where, during alterPartitionReassignments, the replication factor (RF) could end up
larger than the original RF. However, I would like to confirm if 
my understanding is correct.

Thank you!

--
Best,
Kuan-Po

On 2022/07/28 08:59:18 Stanislav Kozlovski wrote:
> Hey all,
> 
> I'd like to start a discussion on a proposal to help API users from
> inadvertently increasing the replication factor of a topic through
> the alter partition reassignments API. The KIP describes two fairly
> easy-to-hit race conditions in which this can happen.
> 
> The KIP itself is pretty simple, yet has a couple of alternatives that can
> help solve the same problem. I would appreciate thoughts from the community
> on how you think we should proceed, and whether the proposal makes sense in
> the first place.
> 
> Thanks!
> 
> KIP:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> 
> -- 
> Best,
> Stanislav
> 

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Stanislav Kozlovski <st...@confluent.io.INVALID>]

Thanks David,

I do prefer the `disallow-replication-factor-change` flag but only for the
CLI. I assume that's what you're proposing instead of
"disable-replication-factor-change". The wording is more natural in your
suggestion I feel.
If we were to modify more (e.g RPC, Admin API), I think it'd be a bit less
straightforward in the code and API to reason about having a
double-negative - e.g `disallowReplicationFactorChange=false`
I have changed the KIP to mention "disallow".

As for the RPCs, I had only envisioned changes in the request - hence I had
only pasted the `.diff`. I have now added the option to be returned as part
of the response too, and have described both RPC jsons in the conventional
way we do it.
Hopefully, this looks good.

I'll be starting a voting thread now.

Best,
Stanislav

On Tue, Aug 16, 2022 at 6:17 AM David Jacot <dj...@confluent.io.invalid>
wrote:

> Thanks Stan. Overall, the KIP looks good to me. I have two minor comments:
>
> * Could we document the different versions in the
> AlterPartitionReassignmentsRequest/Request schema? You can look at
> other requests/responses to see how we have done this so far.
>
> * I wonder if --disallow-replication-factor-change would be a better
> name. I don't feel strong about this so I am happy to go with the
> quorum here.
>
> Best,
> David
>
> On Tue, Aug 16, 2022 at 12:31 AM Stanislav Kozlovski
> <st...@confluent.io.invalid> wrote:
> >
> > Thanks for the discussion all,
> >
> > I have updated the KIP to mention throwing an UnsupportedVersionException
> > if the server is running an old version that would not honor the
> configured
> > allowReplicationFactor option.
> >
> > Please take a look:
> > - KIP:
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > - changes:
> >
> https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=217392873&selectedPageVersions=4&selectedPageVersions=3
> >
> > If there aren't extra comments, I plan on starting a vote thread by the
> end
> > of this week.
> >
> > Best,
> > Stanislav
> >
> > On Tue, Aug 9, 2022 at 5:06 AM David Jacot <dj...@confluent.io.invalid>
> > wrote:
> >
> > > Throwing an UnsupportedVersionException with an appropriate message
> > > seems to be the best option when the new API is not supported and
> > > AllowReplicationFactorChange is not set to the default value.
> > >
> > > Cheers,
> > > David
> > >
> > > On Mon, Aug 8, 2022 at 6:25 PM Vikas Singh <vikas@confluent.io.invalid
> >
> > > wrote:
> > > >
> > > > I personally like the UVE option. It provides options for clients to
> go
> > > > either way, retry or abort. If we do it in AdminClient, then users
> have
> > > to
> > > > live with what we have chosen.
> > > >
> > > > > Note this can happen during an RF change too. e.g [1,2,3] =>
> [4,5,6,7]
> > > (RF
> > > > > change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
> > > > > reassignment to [9,10,11], the logic will compare [4,5,6,7] to
> > > [9,10,11].
> > > > > In such a situation where one wants to cancel the RF increase and
> > > reassign
> > > > > again, one first needs to cancel the existing reassignment via the
> API
> > > (no
> > > > > special action required despite RF change)
> > > >
> > > > Thanks for the explanation. I did realize this nuance and thus
> requested
> > > to
> > > > put that in KIP as it's not mentioned why the choice was made. I am
> fine
> > > if
> > > > you choose to not do it in the interest of brevity.
> > > >
> > > > Vikas
> > > >
> > > > On Sun, Aug 7, 2022 at 9:02 AM Stanislav Kozlovski
> > > > <st...@confluent.io.invalid> wrote:
> > > >
> > > > > Thank you for the reviews.
> > > > >
> > > > > Vikas,
> > > > > > > In the case of an already-reassigning partition being
> reassigned
> > > again,
> > > > > the validation compares the targetReplicaSet size of the
> reassignment
> > > to
> > > > > the targetReplicaSet size of the new reassignment and throws if
> those
> > > > > differ.
> > > > > > Can you add more detail to this, or clarify what is
> targetReplicaSet
> > > (for
> > > > > e.g. why not sourceReplicaSet?) and how the target replica set
> will be
> > > > > calculated?
> > > > > If a reassignment is ongoing, such that [1,2,3] => [4,5,6] (the
> > > replica set
> > > > > in Kafka will be [1,2,3,4,5,6] during the reassignment), and you
> try to
> > > > > issue a new reassignment (e.g [7,8,9], Kafka should NOT think that
> the
> > > RF
> > > > > of the partition is 6 just because a reassignment is ongoing.
> Hence, we
> > > > > compare [4,5,6]'s length to [7,8,9]
> > > > > The targetReplicaSet is a term we use in KIP-455
> > > > > <
> > > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-455%3A+Create+an+Administrative+API+for+Replica+Reassignment
> > > > > >.
> > > > > It means the desired replica set that a given reassignment is
> trying to
> > > > > achieve. Here we compare said set of the on-going reassignment to
> the
> > > new
> > > > > reassignment.
> > > > >
> > > > > Note this can happen during an RF change too. e.g [1,2,3] =>
> [4,5,6,7]
> > > (RF
> > > > > change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
> > > > > reassignment to [9,10,11], the logic will compare [4,5,6,7] to
> > > [9,10,11].
> > > > > In such a situation where one wants to cancel the RF increase and
> > > reassign
> > > > > again, one first needs to cancel the existing reassignment via the
> API
> > > (no
> > > > > special action required despite RF change)
> > > > >
> > > > >
> > > > > > And what about the reassign partitions CLI? Do we want to expose
> the
> > > > > option there too?
> > > > > Yes, this is already present in the KIP if I'm not mistaken. We
> > > describe it
> > > > > in "Accordingly, the kafka-reassign-partitions.sh tool will be
> updated
> > > to
> > > > > allow supplying the new option:"
> > > > > I have edited the KIP to contain two clear paragraphs called Admin
> API
> > > and
> > > > > CLI now.
> > > > >
> > > > > Colin,
> > > > >
> > > > > >  it would be nice for the first paragraph to be a bit more
> explicit
> > > about
> > > > > this goal.
> > > > > sounds good, updated it with that suggestion.
> > > > >
> > > > > > client-side forward compatibility
> > > > > I was under the assumption that it is not recommended to upgrade
> > > clients
> > > > > before brokers, but a quick search cleared it up to me that we're
> > > pretty
> > > > > intentional about allowing that
> > > > > <
> > > > >
> > >
> https://www.confluent.io/blog/upgrading-apache-kafka-clients-just-got-easier/
> > > > > >
> > > > > .
> > > > > Do you happen to know if we have any policy on client-side forward
> > > > > compatibility with regard to such things -- extending "write" APIs
> > > (that
> > > > > mutate the state) with fields that conditionally limit that
> > > modification?
> > > > > It seems like a rare use case to me, hence renaming it to something
> > > like
> > > > > tryDisableReplicationFactorChange may unnecessary impair the API.
> > > > >
> > > > > Would Admin API documentation that says "this is supported only
> from
> > > > > brokers on version X and above. If not supported, the default
> behavior
> > > (no
> > > > > replication factor guards) is applied" be sufficient?
> > > > >
> > > > > Best,
> > > > > Stanislav
> > > > >
> > > > >
> > > > > On Fri, Aug 5, 2022 at 8:32 PM Colin McCabe <cm...@apache.org>
> > > wrote:
> > > > >
> > > > > > Hi Stanislav,
> > > > > >
> > > > > > Thanks for the KIP. I think this is a nice solution to the
> problem
> > > of not
> > > > > > wanting to change the replication factor during reassignments.
> > > > > >
> > > > > > Just from a writing point of view, it would be nice for the first
> > > > > > paragraph to be a bit more explicit about this goal. Maybe lead
> with
> > > > > "Many
> > > > > > times, we don't want to change the replication factor of a
> partition
> > > > > during
> > > > > > reassignment..." As it is, we're talking about metadata races
> before
> > > > > we've
> > > > > > even explained what the goal is that the metadata races are
> > > thwarting. :)
> > > > > >
> > > > > > I like the RPC and command-line format changes; they are
> well-done
> > > and
> > > > > > well-written. One thing we do need to spell out, those, is what
> the
> > > > > > behavior is when the server does not support this new option. The
> > > > > simplest
> > > > > > thing to do would be for the client to throw
> > > UnsupportedVersionException
> > > > > > with an exception message indicating what the problem is. Then
> the
> > > caller
> > > > > > could catch this and re-try the call without the flag (or give
> up, as
> > > > > > appropriate?)
> > > > > >
> > > > > > The other option is to continue on but not actually protect
> > > replication
> > > > > > factor. If we do this, at minimum we'd need to rename the flag
> > > something
> > > > > > like "try to protect replication factor" to make it clear that
> it's
> > > > > > best-effort.
> > > > > >
> > > > > > It's sort of debatable which way is better. In principle the UVE
> > > sounds
> > > > > > nicer, but in practice maybe the other behavior is best? I
> suspect
> > > most
> > > > > > systems would turn around and retry without the flag in the
> event of
> > > a
> > > > > > UVE...
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > >
> > > > > > On Thu, Aug 4, 2022, at 13:37, Vikas Singh wrote:
> > > > > > > Thanks Stanislav for the KIP. Seems like a reasonable proposal,
> > > > > > > preventing users from accidentally altering the replica set
> under
> > > > > certain
> > > > > > > conditions. I have couple of comments:
> > > > > > >
> > > > > > >
> > > > > > >> In the case of an already-reassigning partition being
> reassigned
> > > > > again,
> > > > > > > the validation compares the targetReplicaSet size of the
> > > reassignment
> > > > > to
> > > > > > > the targetReplicaSet size of the new reassignment and throws if
> > > those
> > > > > > > differ.
> > > > > > > Can you add more detail to this, or clarify what is
> > > targetReplicaSet
> > > > > (for
> > > > > > > e.g. why not sourceReplicaSet?) and how the target replica set
> > > will be
> > > > > > > calculated?
> > > > > > >
> > > > > > > And what about the reassign partitions CLI? Do we want to
> expose
> > > the
> > > > > > option
> > > > > > > there too?
> > > > > > >
> > > > > > > Cheers,
> > > > > > > Vikas
> > > > > > >
> > > > > > > On Thu, Jul 28, 2022 at 1:59 AM Stanislav Kozlovski <
> > > > > > stanislav@confluent.io>
> > > > > > > wrote:
> > > > > > >
> > > > > > >> Hey all,
> > > > > > >>
> > > > > > >> I'd like to start a discussion on a proposal to help API users
> > > from
> > > > > > >> inadvertently increasing the replication factor of a topic
> through
> > > > > > >> the alter partition reassignments API. The KIP describes two
> > > fairly
> > > > > > >> easy-to-hit race conditions in which this can happen.
> > > > > > >>
> > > > > > >> The KIP itself is pretty simple, yet has a couple of
> alternatives
> > > that
> > > > > > can
> > > > > > >> help solve the same problem. I would appreciate thoughts from
> the
> > > > > > community
> > > > > > >> on how you think we should proceed, and whether the proposal
> makes
> > > > > > sense in
> > > > > > >> the first place.
> > > > > > >>
> > > > > > >> Thanks!
> > > > > > >>
> > > > > > >> KIP:
> > > > > > >>
> > > > > >
> > > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > > > > > >> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> > > > > > >>
> > > > > > >> --
> > > > > > >> Best,
> > > > > > >> Stanislav
> > > > > > >>
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Best,
> > > > > Stanislav
> > > > >
> > >
> >
> >
> > --
> > Best,
> > Stanislav
>


-- 
Best,
Stanislav

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[David Jacot <dj...@confluent.io.INVALID>]

Thanks Stan. Overall, the KIP looks good to me. I have two minor comments:

* Could we document the different versions in the
AlterPartitionReassignmentsRequest/Request schema? You can look at
other requests/responses to see how we have done this so far.

* I wonder if --disallow-replication-factor-change would be a better
name. I don't feel strong about this so I am happy to go with the
quorum here.

Best,
David

On Tue, Aug 16, 2022 at 12:31 AM Stanislav Kozlovski
<st...@confluent.io.invalid> wrote:
>
> Thanks for the discussion all,
>
> I have updated the KIP to mention throwing an UnsupportedVersionException
> if the server is running an old version that would not honor the configured
> allowReplicationFactor option.
>
> Please take a look:
> - KIP:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> - changes:
> https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=217392873&selectedPageVersions=4&selectedPageVersions=3
>
> If there aren't extra comments, I plan on starting a vote thread by the end
> of this week.
>
> Best,
> Stanislav
>
> On Tue, Aug 9, 2022 at 5:06 AM David Jacot <dj...@confluent.io.invalid>
> wrote:
>
> > Throwing an UnsupportedVersionException with an appropriate message
> > seems to be the best option when the new API is not supported and
> > AllowReplicationFactorChange is not set to the default value.
> >
> > Cheers,
> > David
> >
> > On Mon, Aug 8, 2022 at 6:25 PM Vikas Singh <vi...@confluent.io.invalid>
> > wrote:
> > >
> > > I personally like the UVE option. It provides options for clients to go
> > > either way, retry or abort. If we do it in AdminClient, then users have
> > to
> > > live with what we have chosen.
> > >
> > > > Note this can happen during an RF change too. e.g [1,2,3] => [4,5,6,7]
> > (RF
> > > > change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
> > > > reassignment to [9,10,11], the logic will compare [4,5,6,7] to
> > [9,10,11].
> > > > In such a situation where one wants to cancel the RF increase and
> > reassign
> > > > again, one first needs to cancel the existing reassignment via the API
> > (no
> > > > special action required despite RF change)
> > >
> > > Thanks for the explanation. I did realize this nuance and thus requested
> > to
> > > put that in KIP as it's not mentioned why the choice was made. I am fine
> > if
> > > you choose to not do it in the interest of brevity.
> > >
> > > Vikas
> > >
> > > On Sun, Aug 7, 2022 at 9:02 AM Stanislav Kozlovski
> > > <st...@confluent.io.invalid> wrote:
> > >
> > > > Thank you for the reviews.
> > > >
> > > > Vikas,
> > > > > > In the case of an already-reassigning partition being reassigned
> > again,
> > > > the validation compares the targetReplicaSet size of the reassignment
> > to
> > > > the targetReplicaSet size of the new reassignment and throws if those
> > > > differ.
> > > > > Can you add more detail to this, or clarify what is targetReplicaSet
> > (for
> > > > e.g. why not sourceReplicaSet?) and how the target replica set will be
> > > > calculated?
> > > > If a reassignment is ongoing, such that [1,2,3] => [4,5,6] (the
> > replica set
> > > > in Kafka will be [1,2,3,4,5,6] during the reassignment), and you try to
> > > > issue a new reassignment (e.g [7,8,9], Kafka should NOT think that the
> > RF
> > > > of the partition is 6 just because a reassignment is ongoing. Hence, we
> > > > compare [4,5,6]'s length to [7,8,9]
> > > > The targetReplicaSet is a term we use in KIP-455
> > > > <
> > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-455%3A+Create+an+Administrative+API+for+Replica+Reassignment
> > > > >.
> > > > It means the desired replica set that a given reassignment is trying to
> > > > achieve. Here we compare said set of the on-going reassignment to the
> > new
> > > > reassignment.
> > > >
> > > > Note this can happen during an RF change too. e.g [1,2,3] => [4,5,6,7]
> > (RF
> > > > change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
> > > > reassignment to [9,10,11], the logic will compare [4,5,6,7] to
> > [9,10,11].
> > > > In such a situation where one wants to cancel the RF increase and
> > reassign
> > > > again, one first needs to cancel the existing reassignment via the API
> > (no
> > > > special action required despite RF change)
> > > >
> > > >
> > > > > And what about the reassign partitions CLI? Do we want to expose the
> > > > option there too?
> > > > Yes, this is already present in the KIP if I'm not mistaken. We
> > describe it
> > > > in "Accordingly, the kafka-reassign-partitions.sh tool will be updated
> > to
> > > > allow supplying the new option:"
> > > > I have edited the KIP to contain two clear paragraphs called Admin API
> > and
> > > > CLI now.
> > > >
> > > > Colin,
> > > >
> > > > >  it would be nice for the first paragraph to be a bit more explicit
> > about
> > > > this goal.
> > > > sounds good, updated it with that suggestion.
> > > >
> > > > > client-side forward compatibility
> > > > I was under the assumption that it is not recommended to upgrade
> > clients
> > > > before brokers, but a quick search cleared it up to me that we're
> > pretty
> > > > intentional about allowing that
> > > > <
> > > >
> > https://www.confluent.io/blog/upgrading-apache-kafka-clients-just-got-easier/
> > > > >
> > > > .
> > > > Do you happen to know if we have any policy on client-side forward
> > > > compatibility with regard to such things -- extending "write" APIs
> > (that
> > > > mutate the state) with fields that conditionally limit that
> > modification?
> > > > It seems like a rare use case to me, hence renaming it to something
> > like
> > > > tryDisableReplicationFactorChange may unnecessary impair the API.
> > > >
> > > > Would Admin API documentation that says "this is supported only from
> > > > brokers on version X and above. If not supported, the default behavior
> > (no
> > > > replication factor guards) is applied" be sufficient?
> > > >
> > > > Best,
> > > > Stanislav
> > > >
> > > >
> > > > On Fri, Aug 5, 2022 at 8:32 PM Colin McCabe <cm...@apache.org>
> > wrote:
> > > >
> > > > > Hi Stanislav,
> > > > >
> > > > > Thanks for the KIP. I think this is a nice solution to the problem
> > of not
> > > > > wanting to change the replication factor during reassignments.
> > > > >
> > > > > Just from a writing point of view, it would be nice for the first
> > > > > paragraph to be a bit more explicit about this goal. Maybe lead with
> > > > "Many
> > > > > times, we don't want to change the replication factor of a partition
> > > > during
> > > > > reassignment..." As it is, we're talking about metadata races before
> > > > we've
> > > > > even explained what the goal is that the metadata races are
> > thwarting. :)
> > > > >
> > > > > I like the RPC and command-line format changes; they are well-done
> > and
> > > > > well-written. One thing we do need to spell out, those, is what the
> > > > > behavior is when the server does not support this new option. The
> > > > simplest
> > > > > thing to do would be for the client to throw
> > UnsupportedVersionException
> > > > > with an exception message indicating what the problem is. Then the
> > caller
> > > > > could catch this and re-try the call without the flag (or give up, as
> > > > > appropriate?)
> > > > >
> > > > > The other option is to continue on but not actually protect
> > replication
> > > > > factor. If we do this, at minimum we'd need to rename the flag
> > something
> > > > > like "try to protect replication factor" to make it clear that it's
> > > > > best-effort.
> > > > >
> > > > > It's sort of debatable which way is better. In principle the UVE
> > sounds
> > > > > nicer, but in practice maybe the other behavior is best? I suspect
> > most
> > > > > systems would turn around and retry without the flag in the event of
> > a
> > > > > UVE...
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > On Thu, Aug 4, 2022, at 13:37, Vikas Singh wrote:
> > > > > > Thanks Stanislav for the KIP. Seems like a reasonable proposal,
> > > > > > preventing users from accidentally altering the replica set under
> > > > certain
> > > > > > conditions. I have couple of comments:
> > > > > >
> > > > > >
> > > > > >> In the case of an already-reassigning partition being reassigned
> > > > again,
> > > > > > the validation compares the targetReplicaSet size of the
> > reassignment
> > > > to
> > > > > > the targetReplicaSet size of the new reassignment and throws if
> > those
> > > > > > differ.
> > > > > > Can you add more detail to this, or clarify what is
> > targetReplicaSet
> > > > (for
> > > > > > e.g. why not sourceReplicaSet?) and how the target replica set
> > will be
> > > > > > calculated?
> > > > > >
> > > > > > And what about the reassign partitions CLI? Do we want to expose
> > the
> > > > > option
> > > > > > there too?
> > > > > >
> > > > > > Cheers,
> > > > > > Vikas
> > > > > >
> > > > > > On Thu, Jul 28, 2022 at 1:59 AM Stanislav Kozlovski <
> > > > > stanislav@confluent.io>
> > > > > > wrote:
> > > > > >
> > > > > >> Hey all,
> > > > > >>
> > > > > >> I'd like to start a discussion on a proposal to help API users
> > from
> > > > > >> inadvertently increasing the replication factor of a topic through
> > > > > >> the alter partition reassignments API. The KIP describes two
> > fairly
> > > > > >> easy-to-hit race conditions in which this can happen.
> > > > > >>
> > > > > >> The KIP itself is pretty simple, yet has a couple of alternatives
> > that
> > > > > can
> > > > > >> help solve the same problem. I would appreciate thoughts from the
> > > > > community
> > > > > >> on how you think we should proceed, and whether the proposal makes
> > > > > sense in
> > > > > >> the first place.
> > > > > >>
> > > > > >> Thanks!
> > > > > >>
> > > > > >> KIP:
> > > > > >>
> > > > >
> > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > > > > >> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> > > > > >>
> > > > > >> --
> > > > > >> Best,
> > > > > >> Stanislav
> > > > > >>
> > > > >
> > > >
> > > >
> > > > --
> > > > Best,
> > > > Stanislav
> > > >
> >
>
>
> --
> Best,
> Stanislav

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Stanislav Kozlovski <st...@confluent.io.INVALID>]

Thanks for the discussion all,

I have updated the KIP to mention throwing an UnsupportedVersionException
if the server is running an old version that would not honor the configured
allowReplicationFactor option.

Please take a look:
- KIP:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
- changes:
https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=217392873&selectedPageVersions=4&selectedPageVersions=3

If there aren't extra comments, I plan on starting a vote thread by the end
of this week.

Best,
Stanislav

On Tue, Aug 9, 2022 at 5:06 AM David Jacot <dj...@confluent.io.invalid>
wrote:

> Throwing an UnsupportedVersionException with an appropriate message
> seems to be the best option when the new API is not supported and
> AllowReplicationFactorChange is not set to the default value.
>
> Cheers,
> David
>
> On Mon, Aug 8, 2022 at 6:25 PM Vikas Singh <vi...@confluent.io.invalid>
> wrote:
> >
> > I personally like the UVE option. It provides options for clients to go
> > either way, retry or abort. If we do it in AdminClient, then users have
> to
> > live with what we have chosen.
> >
> > > Note this can happen during an RF change too. e.g [1,2,3] => [4,5,6,7]
> (RF
> > > change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
> > > reassignment to [9,10,11], the logic will compare [4,5,6,7] to
> [9,10,11].
> > > In such a situation where one wants to cancel the RF increase and
> reassign
> > > again, one first needs to cancel the existing reassignment via the API
> (no
> > > special action required despite RF change)
> >
> > Thanks for the explanation. I did realize this nuance and thus requested
> to
> > put that in KIP as it's not mentioned why the choice was made. I am fine
> if
> > you choose to not do it in the interest of brevity.
> >
> > Vikas
> >
> > On Sun, Aug 7, 2022 at 9:02 AM Stanislav Kozlovski
> > <st...@confluent.io.invalid> wrote:
> >
> > > Thank you for the reviews.
> > >
> > > Vikas,
> > > > > In the case of an already-reassigning partition being reassigned
> again,
> > > the validation compares the targetReplicaSet size of the reassignment
> to
> > > the targetReplicaSet size of the new reassignment and throws if those
> > > differ.
> > > > Can you add more detail to this, or clarify what is targetReplicaSet
> (for
> > > e.g. why not sourceReplicaSet?) and how the target replica set will be
> > > calculated?
> > > If a reassignment is ongoing, such that [1,2,3] => [4,5,6] (the
> replica set
> > > in Kafka will be [1,2,3,4,5,6] during the reassignment), and you try to
> > > issue a new reassignment (e.g [7,8,9], Kafka should NOT think that the
> RF
> > > of the partition is 6 just because a reassignment is ongoing. Hence, we
> > > compare [4,5,6]'s length to [7,8,9]
> > > The targetReplicaSet is a term we use in KIP-455
> > > <
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-455%3A+Create+an+Administrative+API+for+Replica+Reassignment
> > > >.
> > > It means the desired replica set that a given reassignment is trying to
> > > achieve. Here we compare said set of the on-going reassignment to the
> new
> > > reassignment.
> > >
> > > Note this can happen during an RF change too. e.g [1,2,3] => [4,5,6,7]
> (RF
> > > change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
> > > reassignment to [9,10,11], the logic will compare [4,5,6,7] to
> [9,10,11].
> > > In such a situation where one wants to cancel the RF increase and
> reassign
> > > again, one first needs to cancel the existing reassignment via the API
> (no
> > > special action required despite RF change)
> > >
> > >
> > > > And what about the reassign partitions CLI? Do we want to expose the
> > > option there too?
> > > Yes, this is already present in the KIP if I'm not mistaken. We
> describe it
> > > in "Accordingly, the kafka-reassign-partitions.sh tool will be updated
> to
> > > allow supplying the new option:"
> > > I have edited the KIP to contain two clear paragraphs called Admin API
> and
> > > CLI now.
> > >
> > > Colin,
> > >
> > > >  it would be nice for the first paragraph to be a bit more explicit
> about
> > > this goal.
> > > sounds good, updated it with that suggestion.
> > >
> > > > client-side forward compatibility
> > > I was under the assumption that it is not recommended to upgrade
> clients
> > > before brokers, but a quick search cleared it up to me that we're
> pretty
> > > intentional about allowing that
> > > <
> > >
> https://www.confluent.io/blog/upgrading-apache-kafka-clients-just-got-easier/
> > > >
> > > .
> > > Do you happen to know if we have any policy on client-side forward
> > > compatibility with regard to such things -- extending "write" APIs
> (that
> > > mutate the state) with fields that conditionally limit that
> modification?
> > > It seems like a rare use case to me, hence renaming it to something
> like
> > > tryDisableReplicationFactorChange may unnecessary impair the API.
> > >
> > > Would Admin API documentation that says "this is supported only from
> > > brokers on version X and above. If not supported, the default behavior
> (no
> > > replication factor guards) is applied" be sufficient?
> > >
> > > Best,
> > > Stanislav
> > >
> > >
> > > On Fri, Aug 5, 2022 at 8:32 PM Colin McCabe <cm...@apache.org>
> wrote:
> > >
> > > > Hi Stanislav,
> > > >
> > > > Thanks for the KIP. I think this is a nice solution to the problem
> of not
> > > > wanting to change the replication factor during reassignments.
> > > >
> > > > Just from a writing point of view, it would be nice for the first
> > > > paragraph to be a bit more explicit about this goal. Maybe lead with
> > > "Many
> > > > times, we don't want to change the replication factor of a partition
> > > during
> > > > reassignment..." As it is, we're talking about metadata races before
> > > we've
> > > > even explained what the goal is that the metadata races are
> thwarting. :)
> > > >
> > > > I like the RPC and command-line format changes; they are well-done
> and
> > > > well-written. One thing we do need to spell out, those, is what the
> > > > behavior is when the server does not support this new option. The
> > > simplest
> > > > thing to do would be for the client to throw
> UnsupportedVersionException
> > > > with an exception message indicating what the problem is. Then the
> caller
> > > > could catch this and re-try the call without the flag (or give up, as
> > > > appropriate?)
> > > >
> > > > The other option is to continue on but not actually protect
> replication
> > > > factor. If we do this, at minimum we'd need to rename the flag
> something
> > > > like "try to protect replication factor" to make it clear that it's
> > > > best-effort.
> > > >
> > > > It's sort of debatable which way is better. In principle the UVE
> sounds
> > > > nicer, but in practice maybe the other behavior is best? I suspect
> most
> > > > systems would turn around and retry without the flag in the event of
> a
> > > > UVE...
> > > >
> > > > best,
> > > > Colin
> > > >
> > > >
> > > > On Thu, Aug 4, 2022, at 13:37, Vikas Singh wrote:
> > > > > Thanks Stanislav for the KIP. Seems like a reasonable proposal,
> > > > > preventing users from accidentally altering the replica set under
> > > certain
> > > > > conditions. I have couple of comments:
> > > > >
> > > > >
> > > > >> In the case of an already-reassigning partition being reassigned
> > > again,
> > > > > the validation compares the targetReplicaSet size of the
> reassignment
> > > to
> > > > > the targetReplicaSet size of the new reassignment and throws if
> those
> > > > > differ.
> > > > > Can you add more detail to this, or clarify what is
> targetReplicaSet
> > > (for
> > > > > e.g. why not sourceReplicaSet?) and how the target replica set
> will be
> > > > > calculated?
> > > > >
> > > > > And what about the reassign partitions CLI? Do we want to expose
> the
> > > > option
> > > > > there too?
> > > > >
> > > > > Cheers,
> > > > > Vikas
> > > > >
> > > > > On Thu, Jul 28, 2022 at 1:59 AM Stanislav Kozlovski <
> > > > stanislav@confluent.io>
> > > > > wrote:
> > > > >
> > > > >> Hey all,
> > > > >>
> > > > >> I'd like to start a discussion on a proposal to help API users
> from
> > > > >> inadvertently increasing the replication factor of a topic through
> > > > >> the alter partition reassignments API. The KIP describes two
> fairly
> > > > >> easy-to-hit race conditions in which this can happen.
> > > > >>
> > > > >> The KIP itself is pretty simple, yet has a couple of alternatives
> that
> > > > can
> > > > >> help solve the same problem. I would appreciate thoughts from the
> > > > community
> > > > >> on how you think we should proceed, and whether the proposal makes
> > > > sense in
> > > > >> the first place.
> > > > >>
> > > > >> Thanks!
> > > > >>
> > > > >> KIP:
> > > > >>
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > > > >> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> > > > >>
> > > > >> --
> > > > >> Best,
> > > > >> Stanislav
> > > > >>
> > > >
> > >
> > >
> > > --
> > > Best,
> > > Stanislav
> > >
>


-- 
Best,
Stanislav

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[David Jacot <dj...@confluent.io.INVALID>]

Throwing an UnsupportedVersionException with an appropriate message
seems to be the best option when the new API is not supported and
AllowReplicationFactorChange is not set to the default value.

Cheers,
David

On Mon, Aug 8, 2022 at 6:25 PM Vikas Singh <vi...@confluent.io.invalid> wrote:
>
> I personally like the UVE option. It provides options for clients to go
> either way, retry or abort. If we do it in AdminClient, then users have to
> live with what we have chosen.
>
> > Note this can happen during an RF change too. e.g [1,2,3] => [4,5,6,7] (RF
> > change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
> > reassignment to [9,10,11], the logic will compare [4,5,6,7] to [9,10,11].
> > In such a situation where one wants to cancel the RF increase and reassign
> > again, one first needs to cancel the existing reassignment via the API (no
> > special action required despite RF change)
>
> Thanks for the explanation. I did realize this nuance and thus requested to
> put that in KIP as it's not mentioned why the choice was made. I am fine if
> you choose to not do it in the interest of brevity.
>
> Vikas
>
> On Sun, Aug 7, 2022 at 9:02 AM Stanislav Kozlovski
> <st...@confluent.io.invalid> wrote:
>
> > Thank you for the reviews.
> >
> > Vikas,
> > > > In the case of an already-reassigning partition being reassigned again,
> > the validation compares the targetReplicaSet size of the reassignment to
> > the targetReplicaSet size of the new reassignment and throws if those
> > differ.
> > > Can you add more detail to this, or clarify what is targetReplicaSet (for
> > e.g. why not sourceReplicaSet?) and how the target replica set will be
> > calculated?
> > If a reassignment is ongoing, such that [1,2,3] => [4,5,6] (the replica set
> > in Kafka will be [1,2,3,4,5,6] during the reassignment), and you try to
> > issue a new reassignment (e.g [7,8,9], Kafka should NOT think that the RF
> > of the partition is 6 just because a reassignment is ongoing. Hence, we
> > compare [4,5,6]'s length to [7,8,9]
> > The targetReplicaSet is a term we use in KIP-455
> > <
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-455%3A+Create+an+Administrative+API+for+Replica+Reassignment
> > >.
> > It means the desired replica set that a given reassignment is trying to
> > achieve. Here we compare said set of the on-going reassignment to the new
> > reassignment.
> >
> > Note this can happen during an RF change too. e.g [1,2,3] => [4,5,6,7] (RF
> > change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
> > reassignment to [9,10,11], the logic will compare [4,5,6,7] to [9,10,11].
> > In such a situation where one wants to cancel the RF increase and reassign
> > again, one first needs to cancel the existing reassignment via the API (no
> > special action required despite RF change)
> >
> >
> > > And what about the reassign partitions CLI? Do we want to expose the
> > option there too?
> > Yes, this is already present in the KIP if I'm not mistaken. We describe it
> > in "Accordingly, the kafka-reassign-partitions.sh tool will be updated to
> > allow supplying the new option:"
> > I have edited the KIP to contain two clear paragraphs called Admin API and
> > CLI now.
> >
> > Colin,
> >
> > >  it would be nice for the first paragraph to be a bit more explicit about
> > this goal.
> > sounds good, updated it with that suggestion.
> >
> > > client-side forward compatibility
> > I was under the assumption that it is not recommended to upgrade clients
> > before brokers, but a quick search cleared it up to me that we're pretty
> > intentional about allowing that
> > <
> > https://www.confluent.io/blog/upgrading-apache-kafka-clients-just-got-easier/
> > >
> > .
> > Do you happen to know if we have any policy on client-side forward
> > compatibility with regard to such things -- extending "write" APIs (that
> > mutate the state) with fields that conditionally limit that modification?
> > It seems like a rare use case to me, hence renaming it to something like
> > tryDisableReplicationFactorChange may unnecessary impair the API.
> >
> > Would Admin API documentation that says "this is supported only from
> > brokers on version X and above. If not supported, the default behavior (no
> > replication factor guards) is applied" be sufficient?
> >
> > Best,
> > Stanislav
> >
> >
> > On Fri, Aug 5, 2022 at 8:32 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > > Hi Stanislav,
> > >
> > > Thanks for the KIP. I think this is a nice solution to the problem of not
> > > wanting to change the replication factor during reassignments.
> > >
> > > Just from a writing point of view, it would be nice for the first
> > > paragraph to be a bit more explicit about this goal. Maybe lead with
> > "Many
> > > times, we don't want to change the replication factor of a partition
> > during
> > > reassignment..." As it is, we're talking about metadata races before
> > we've
> > > even explained what the goal is that the metadata races are thwarting. :)
> > >
> > > I like the RPC and command-line format changes; they are well-done and
> > > well-written. One thing we do need to spell out, those, is what the
> > > behavior is when the server does not support this new option. The
> > simplest
> > > thing to do would be for the client to throw UnsupportedVersionException
> > > with an exception message indicating what the problem is. Then the caller
> > > could catch this and re-try the call without the flag (or give up, as
> > > appropriate?)
> > >
> > > The other option is to continue on but not actually protect replication
> > > factor. If we do this, at minimum we'd need to rename the flag something
> > > like "try to protect replication factor" to make it clear that it's
> > > best-effort.
> > >
> > > It's sort of debatable which way is better. In principle the UVE sounds
> > > nicer, but in practice maybe the other behavior is best? I suspect most
> > > systems would turn around and retry without the flag in the event of a
> > > UVE...
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Thu, Aug 4, 2022, at 13:37, Vikas Singh wrote:
> > > > Thanks Stanislav for the KIP. Seems like a reasonable proposal,
> > > > preventing users from accidentally altering the replica set under
> > certain
> > > > conditions. I have couple of comments:
> > > >
> > > >
> > > >> In the case of an already-reassigning partition being reassigned
> > again,
> > > > the validation compares the targetReplicaSet size of the reassignment
> > to
> > > > the targetReplicaSet size of the new reassignment and throws if those
> > > > differ.
> > > > Can you add more detail to this, or clarify what is targetReplicaSet
> > (for
> > > > e.g. why not sourceReplicaSet?) and how the target replica set will be
> > > > calculated?
> > > >
> > > > And what about the reassign partitions CLI? Do we want to expose the
> > > option
> > > > there too?
> > > >
> > > > Cheers,
> > > > Vikas
> > > >
> > > > On Thu, Jul 28, 2022 at 1:59 AM Stanislav Kozlovski <
> > > stanislav@confluent.io>
> > > > wrote:
> > > >
> > > >> Hey all,
> > > >>
> > > >> I'd like to start a discussion on a proposal to help API users from
> > > >> inadvertently increasing the replication factor of a topic through
> > > >> the alter partition reassignments API. The KIP describes two fairly
> > > >> easy-to-hit race conditions in which this can happen.
> > > >>
> > > >> The KIP itself is pretty simple, yet has a couple of alternatives that
> > > can
> > > >> help solve the same problem. I would appreciate thoughts from the
> > > community
> > > >> on how you think we should proceed, and whether the proposal makes
> > > sense in
> > > >> the first place.
> > > >>
> > > >> Thanks!
> > > >>
> > > >> KIP:
> > > >>
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > > >> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> > > >>
> > > >> --
> > > >> Best,
> > > >> Stanislav
> > > >>
> > >
> >
> >
> > --
> > Best,
> > Stanislav
> >

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Vikas Singh <vi...@confluent.io.INVALID>]

I personally like the UVE option. It provides options for clients to go
either way, retry or abort. If we do it in AdminClient, then users have to
live with what we have chosen.

> Note this can happen during an RF change too. e.g [1,2,3] => [4,5,6,7] (RF
> change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
> reassignment to [9,10,11], the logic will compare [4,5,6,7] to [9,10,11].
> In such a situation where one wants to cancel the RF increase and reassign
> again, one first needs to cancel the existing reassignment via the API (no
> special action required despite RF change)

Thanks for the explanation. I did realize this nuance and thus requested to
put that in KIP as it's not mentioned why the choice was made. I am fine if
you choose to not do it in the interest of brevity.

Vikas

On Sun, Aug 7, 2022 at 9:02 AM Stanislav Kozlovski
<st...@confluent.io.invalid> wrote:

> Thank you for the reviews.
>
> Vikas,
> > > In the case of an already-reassigning partition being reassigned again,
> the validation compares the targetReplicaSet size of the reassignment to
> the targetReplicaSet size of the new reassignment and throws if those
> differ.
> > Can you add more detail to this, or clarify what is targetReplicaSet (for
> e.g. why not sourceReplicaSet?) and how the target replica set will be
> calculated?
> If a reassignment is ongoing, such that [1,2,3] => [4,5,6] (the replica set
> in Kafka will be [1,2,3,4,5,6] during the reassignment), and you try to
> issue a new reassignment (e.g [7,8,9], Kafka should NOT think that the RF
> of the partition is 6 just because a reassignment is ongoing. Hence, we
> compare [4,5,6]'s length to [7,8,9]
> The targetReplicaSet is a term we use in KIP-455
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-455%3A+Create+an+Administrative+API+for+Replica+Reassignment
> >.
> It means the desired replica set that a given reassignment is trying to
> achieve. Here we compare said set of the on-going reassignment to the new
> reassignment.
>
> Note this can happen during an RF change too. e.g [1,2,3] => [4,5,6,7] (RF
> change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
> reassignment to [9,10,11], the logic will compare [4,5,6,7] to [9,10,11].
> In such a situation where one wants to cancel the RF increase and reassign
> again, one first needs to cancel the existing reassignment via the API (no
> special action required despite RF change)
>
>
> > And what about the reassign partitions CLI? Do we want to expose the
> option there too?
> Yes, this is already present in the KIP if I'm not mistaken. We describe it
> in "Accordingly, the kafka-reassign-partitions.sh tool will be updated to
> allow supplying the new option:"
> I have edited the KIP to contain two clear paragraphs called Admin API and
> CLI now.
>
> Colin,
>
> >  it would be nice for the first paragraph to be a bit more explicit about
> this goal.
> sounds good, updated it with that suggestion.
>
> > client-side forward compatibility
> I was under the assumption that it is not recommended to upgrade clients
> before brokers, but a quick search cleared it up to me that we're pretty
> intentional about allowing that
> <
> https://www.confluent.io/blog/upgrading-apache-kafka-clients-just-got-easier/
> >
> .
> Do you happen to know if we have any policy on client-side forward
> compatibility with regard to such things -- extending "write" APIs (that
> mutate the state) with fields that conditionally limit that modification?
> It seems like a rare use case to me, hence renaming it to something like
> tryDisableReplicationFactorChange may unnecessary impair the API.
>
> Would Admin API documentation that says "this is supported only from
> brokers on version X and above. If not supported, the default behavior (no
> replication factor guards) is applied" be sufficient?
>
> Best,
> Stanislav
>
>
> On Fri, Aug 5, 2022 at 8:32 PM Colin McCabe <cm...@apache.org> wrote:
>
> > Hi Stanislav,
> >
> > Thanks for the KIP. I think this is a nice solution to the problem of not
> > wanting to change the replication factor during reassignments.
> >
> > Just from a writing point of view, it would be nice for the first
> > paragraph to be a bit more explicit about this goal. Maybe lead with
> "Many
> > times, we don't want to change the replication factor of a partition
> during
> > reassignment..." As it is, we're talking about metadata races before
> we've
> > even explained what the goal is that the metadata races are thwarting. :)
> >
> > I like the RPC and command-line format changes; they are well-done and
> > well-written. One thing we do need to spell out, those, is what the
> > behavior is when the server does not support this new option. The
> simplest
> > thing to do would be for the client to throw UnsupportedVersionException
> > with an exception message indicating what the problem is. Then the caller
> > could catch this and re-try the call without the flag (or give up, as
> > appropriate?)
> >
> > The other option is to continue on but not actually protect replication
> > factor. If we do this, at minimum we'd need to rename the flag something
> > like "try to protect replication factor" to make it clear that it's
> > best-effort.
> >
> > It's sort of debatable which way is better. In principle the UVE sounds
> > nicer, but in practice maybe the other behavior is best? I suspect most
> > systems would turn around and retry without the flag in the event of a
> > UVE...
> >
> > best,
> > Colin
> >
> >
> > On Thu, Aug 4, 2022, at 13:37, Vikas Singh wrote:
> > > Thanks Stanislav for the KIP. Seems like a reasonable proposal,
> > > preventing users from accidentally altering the replica set under
> certain
> > > conditions. I have couple of comments:
> > >
> > >
> > >> In the case of an already-reassigning partition being reassigned
> again,
> > > the validation compares the targetReplicaSet size of the reassignment
> to
> > > the targetReplicaSet size of the new reassignment and throws if those
> > > differ.
> > > Can you add more detail to this, or clarify what is targetReplicaSet
> (for
> > > e.g. why not sourceReplicaSet?) and how the target replica set will be
> > > calculated?
> > >
> > > And what about the reassign partitions CLI? Do we want to expose the
> > option
> > > there too?
> > >
> > > Cheers,
> > > Vikas
> > >
> > > On Thu, Jul 28, 2022 at 1:59 AM Stanislav Kozlovski <
> > stanislav@confluent.io>
> > > wrote:
> > >
> > >> Hey all,
> > >>
> > >> I'd like to start a discussion on a proposal to help API users from
> > >> inadvertently increasing the replication factor of a topic through
> > >> the alter partition reassignments API. The KIP describes two fairly
> > >> easy-to-hit race conditions in which this can happen.
> > >>
> > >> The KIP itself is pretty simple, yet has a couple of alternatives that
> > can
> > >> help solve the same problem. I would appreciate thoughts from the
> > community
> > >> on how you think we should proceed, and whether the proposal makes
> > sense in
> > >> the first place.
> > >>
> > >> Thanks!
> > >>
> > >> KIP:
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> > >> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> > >>
> > >> --
> > >> Best,
> > >> Stanislav
> > >>
> >
>
>
> --
> Best,
> Stanislav
>

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Stanislav Kozlovski <st...@confluent.io.INVALID>]

Thank you for the reviews.

Vikas,
> > In the case of an already-reassigning partition being reassigned again,
the validation compares the targetReplicaSet size of the reassignment to
the targetReplicaSet size of the new reassignment and throws if those
differ.
> Can you add more detail to this, or clarify what is targetReplicaSet (for
e.g. why not sourceReplicaSet?) and how the target replica set will be
calculated?
If a reassignment is ongoing, such that [1,2,3] => [4,5,6] (the replica set
in Kafka will be [1,2,3,4,5,6] during the reassignment), and you try to
issue a new reassignment (e.g [7,8,9], Kafka should NOT think that the RF
of the partition is 6 just because a reassignment is ongoing. Hence, we
compare [4,5,6]'s length to [7,8,9]
The targetReplicaSet is a term we use in KIP-455
<https://cwiki.apache.org/confluence/display/KAFKA/KIP-455%3A+Create+an+Administrative+API+for+Replica+Reassignment>.
It means the desired replica set that a given reassignment is trying to
achieve. Here we compare said set of the on-going reassignment to the new
reassignment.

Note this can happen during an RF change too. e.g [1,2,3] => [4,5,6,7] (RF
change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a
reassignment to [9,10,11], the logic will compare [4,5,6,7] to [9,10,11].
In such a situation where one wants to cancel the RF increase and reassign
again, one first needs to cancel the existing reassignment via the API (no
special action required despite RF change)


> And what about the reassign partitions CLI? Do we want to expose the
option there too?
Yes, this is already present in the KIP if I'm not mistaken. We describe it
in "Accordingly, the kafka-reassign-partitions.sh tool will be updated to
allow supplying the new option:"
I have edited the KIP to contain two clear paragraphs called Admin API and
CLI now.

Colin,

>  it would be nice for the first paragraph to be a bit more explicit about
this goal.
sounds good, updated it with that suggestion.

> client-side forward compatibility
I was under the assumption that it is not recommended to upgrade clients
before brokers, but a quick search cleared it up to me that we're pretty
intentional about allowing that
<https://www.confluent.io/blog/upgrading-apache-kafka-clients-just-got-easier/>
.
Do you happen to know if we have any policy on client-side forward
compatibility with regard to such things -- extending "write" APIs (that
mutate the state) with fields that conditionally limit that modification?
It seems like a rare use case to me, hence renaming it to something like
tryDisableReplicationFactorChange may unnecessary impair the API.

Would Admin API documentation that says "this is supported only from
brokers on version X and above. If not supported, the default behavior (no
replication factor guards) is applied" be sufficient?

Best,
Stanislav


On Fri, Aug 5, 2022 at 8:32 PM Colin McCabe <cm...@apache.org> wrote:

> Hi Stanislav,
>
> Thanks for the KIP. I think this is a nice solution to the problem of not
> wanting to change the replication factor during reassignments.
>
> Just from a writing point of view, it would be nice for the first
> paragraph to be a bit more explicit about this goal. Maybe lead with "Many
> times, we don't want to change the replication factor of a partition during
> reassignment..." As it is, we're talking about metadata races before we've
> even explained what the goal is that the metadata races are thwarting. :)
>
> I like the RPC and command-line format changes; they are well-done and
> well-written. One thing we do need to spell out, those, is what the
> behavior is when the server does not support this new option. The simplest
> thing to do would be for the client to throw UnsupportedVersionException
> with an exception message indicating what the problem is. Then the caller
> could catch this and re-try the call without the flag (or give up, as
> appropriate?)
>
> The other option is to continue on but not actually protect replication
> factor. If we do this, at minimum we'd need to rename the flag something
> like "try to protect replication factor" to make it clear that it's
> best-effort.
>
> It's sort of debatable which way is better. In principle the UVE sounds
> nicer, but in practice maybe the other behavior is best? I suspect most
> systems would turn around and retry without the flag in the event of a
> UVE...
>
> best,
> Colin
>
>
> On Thu, Aug 4, 2022, at 13:37, Vikas Singh wrote:
> > Thanks Stanislav for the KIP. Seems like a reasonable proposal,
> > preventing users from accidentally altering the replica set under certain
> > conditions. I have couple of comments:
> >
> >
> >> In the case of an already-reassigning partition being reassigned again,
> > the validation compares the targetReplicaSet size of the reassignment to
> > the targetReplicaSet size of the new reassignment and throws if those
> > differ.
> > Can you add more detail to this, or clarify what is targetReplicaSet (for
> > e.g. why not sourceReplicaSet?) and how the target replica set will be
> > calculated?
> >
> > And what about the reassign partitions CLI? Do we want to expose the
> option
> > there too?
> >
> > Cheers,
> > Vikas
> >
> > On Thu, Jul 28, 2022 at 1:59 AM Stanislav Kozlovski <
> stanislav@confluent.io>
> > wrote:
> >
> >> Hey all,
> >>
> >> I'd like to start a discussion on a proposal to help API users from
> >> inadvertently increasing the replication factor of a topic through
> >> the alter partition reassignments API. The KIP describes two fairly
> >> easy-to-hit race conditions in which this can happen.
> >>
> >> The KIP itself is pretty simple, yet has a couple of alternatives that
> can
> >> help solve the same problem. I would appreciate thoughts from the
> community
> >> on how you think we should proceed, and whether the proposal makes
> sense in
> >> the first place.
> >>
> >> Thanks!
> >>
> >> KIP:
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> >> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
> >>
> >> --
> >> Best,
> >> Stanislav
> >>
>


-- 
Best,
Stanislav

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Colin McCabe <cm...@apache.org>]

Hi Stanislav,

Thanks for the KIP. I think this is a nice solution to the problem of not wanting to change the replication factor during reassignments.

Just from a writing point of view, it would be nice for the first paragraph to be a bit more explicit about this goal. Maybe lead with "Many times, we don't want to change the replication factor of a partition during reassignment..." As it is, we're talking about metadata races before we've even explained what the goal is that the metadata races are thwarting. :)

I like the RPC and command-line format changes; they are well-done and well-written. One thing we do need to spell out, those, is what the behavior is when the server does not support this new option. The simplest thing to do would be for the client to throw UnsupportedVersionException with an exception message indicating what the problem is. Then the caller could catch this and re-try the call without the flag (or give up, as appropriate?)

The other option is to continue on but not actually protect replication factor. If we do this, at minimum we'd need to rename the flag something like "try to protect replication factor" to make it clear that it's best-effort.

It's sort of debatable which way is better. In principle the UVE sounds nicer, but in practice maybe the other behavior is best? I suspect most systems would turn around and retry without the flag in the event of a UVE...

best,
Colin


On Thu, Aug 4, 2022, at 13:37, Vikas Singh wrote:
> Thanks Stanislav for the KIP. Seems like a reasonable proposal,
> preventing users from accidentally altering the replica set under certain
> conditions. I have couple of comments:
>
>
>> In the case of an already-reassigning partition being reassigned again,
> the validation compares the targetReplicaSet size of the reassignment to
> the targetReplicaSet size of the new reassignment and throws if those
> differ.
> Can you add more detail to this, or clarify what is targetReplicaSet (for
> e.g. why not sourceReplicaSet?) and how the target replica set will be
> calculated?
>
> And what about the reassign partitions CLI? Do we want to expose the option
> there too?
>
> Cheers,
> Vikas
>
> On Thu, Jul 28, 2022 at 1:59 AM Stanislav Kozlovski <st...@confluent.io>
> wrote:
>
>> Hey all,
>>
>> I'd like to start a discussion on a proposal to help API users from
>> inadvertently increasing the replication factor of a topic through
>> the alter partition reassignments API. The KIP describes two fairly
>> easy-to-hit race conditions in which this can happen.
>>
>> The KIP itself is pretty simple, yet has a couple of alternatives that can
>> help solve the same problem. I would appreciate thoughts from the community
>> on how you think we should proceed, and whether the proposal makes sense in
>> the first place.
>>
>> Thanks!
>>
>> KIP:
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
>> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
>>
>> --
>> Best,
>> Stanislav
>>

Re: [DISCISS] KIP-860: Add client-provided option to guard against unintentional replication factor change during partition reassignments

[Vikas Singh <vi...@confluent.io.INVALID>]

Thanks Stanislav for the KIP. Seems like a reasonable proposal,
preventing users from accidentally altering the replica set under certain
conditions. I have couple of comments:


> In the case of an already-reassigning partition being reassigned again,
the validation compares the targetReplicaSet size of the reassignment to
the targetReplicaSet size of the new reassignment and throws if those
differ.
Can you add more detail to this, or clarify what is targetReplicaSet (for
e.g. why not sourceReplicaSet?) and how the target replica set will be
calculated?

And what about the reassign partitions CLI? Do we want to expose the option
there too?

Cheers,
Vikas

On Thu, Jul 28, 2022 at 1:59 AM Stanislav Kozlovski <st...@confluent.io>
wrote:

> Hey all,
>
> I'd like to start a discussion on a proposal to help API users from
> inadvertently increasing the replication factor of a topic through
> the alter partition reassignments API. The KIP describes two fairly
> easy-to-hit race conditions in which this can happen.
>
> The KIP itself is pretty simple, yet has a couple of alternatives that can
> help solve the same problem. I would appreciate thoughts from the community
> on how you think we should proceed, and whether the proposal makes sense in
> the first place.
>
> Thanks!
>
> KIP:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments
> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121
>
> --
> Best,
> Stanislav
>