Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply: Allow ignoring a specific error returned from updatePartitions() #184

Conversation

shimon-armis
Copy link

@shimon-armis shimon-armis commented Mar 12, 2024

Description

When applying topic's configuration, we'd like to be able avoid failing in case the desired topic's partitions count is smaller than the actual topic's partitions count (on the broker).
We know that Kafka doesn't allow partitions decrease so instead of failing the operation we'd like to
continue without doing anything (and make this operation idempotent, in that aspect).
This is very convenient when trying to apply a batch of topics in which case, we don't want to fail the rest of the batch for one topic that its partitions count configuration is lower than the actual partitions count. (on the broker).

This change is backward compatible, as the default behavior is kept.

@shimon-armis shimon-armis requested a review from a team as a code owner March 12, 2024 18:23
@shimon-armis
Copy link
Author

@yolken-segment @cdignam-segment Hey guys.
Can you plz take a look?

@shimon-armis
Copy link
Author

@hhahn-tw @petedannemann Can you guys please review this one?

cmd/topicctl/subcmd/apply.go Outdated Show resolved Hide resolved
@shimon-armis shimon-armis force-pushed the shimon.allow_ignoring_error_when_partitions_cnt_conf_smaller_than_the_actual_partitions_cnt branch from f2751c0 to 19daf93 Compare March 21, 2024 22:29
Copy link
Contributor

@hhahn-tw hhahn-tw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

@shimon-armis
Copy link
Author

@hhahn-tw Can you please assist with PR check?

@shimon-armis
Copy link
Author

@noorul @jkoelker @mericsson @hhahn-tw Can you guys assist with the PR check failure?
It doesn't seem related to my change.

@jkoelker
Copy link

@noorul @jkoelker @mericsson @hhahn-tw Can you guys assist with the PR check failure? It doesn't seem related to my change.

just rebase, it got removed recently: b83ceba

@shimon-armis shimon-armis force-pushed the shimon.allow_ignoring_error_when_partitions_cnt_conf_smaller_than_the_actual_partitions_cnt branch from 19daf93 to cd951e6 Compare March 27, 2024 14:55
@shimon-armis
Copy link
Author

@jkoelker can you please approve?

pkg/apply/apply.go Outdated Show resolved Hide resolved
pkg/apply/apply.go Outdated Show resolved Hide resolved
When applying topic's configuration, we'd like to be able
avoid failing in case the desired topic's partition count
is smaller than the actual topic's partitions count (on
the broker).
We know that Kafka doesn't allow partitions decrease,
so instead of failing the operation we'd like to
continue without doing anything.
This is very convenient when trying to apply a batch
of topics in which case, we don't want to fail the rest
of the batch for one topic that its partition count
configuration is lower than the actual partitions count.

This change is backward compatible, as the default behavior
is kept.

Signed-off-by: shimon-armis <[email protected]>
@shimon-armis shimon-armis force-pushed the shimon.allow_ignoring_error_when_partitions_cnt_conf_smaller_than_the_actual_partitions_cnt branch from cd951e6 to 6794218 Compare March 27, 2024 18:26
@shimon-armis
Copy link
Author

@jkoelker I've addressed the comment and updated the code. Can you please approve?

@shimon-armis
Copy link
Author

@yolken-segment @cdignam-segment Can you guys approve?
(Looking for a maintainer to approve - not sure who is the maintainer here...)

@shimon-armis
Copy link
Author

@jkoelker Who can merge this?

@shimon-armis
Copy link
Author

@hhahn-tw @jkoelker Who can merge this PR?

@hhahn-tw hhahn-tw merged commit 25a6f37 into segmentio:master Apr 1, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants