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

fix: SandboxKafkaTopicClient should use default replication factor if applicable #8551

Merged
merged 4 commits into from
Jan 6, 2022
Merged

fix: SandboxKafkaTopicClient should use default replication factor if applicable #8551

merged 4 commits into from
Jan 6, 2022

Conversation

mjsax
Copy link
Member

@mjsax mjsax commented Dec 23, 2021

If replication factor -1 is used (implying to use the broker default) the
SandboxKafkaTopicClient created a mocked topic with replication factor 0
instead of using the broker default.

If multiple statement are submitted as once (ie, via a file), later
statement could inherit the incorrect replication factor and thus fail,
which fails all statements in the file.

Closes #4800

…applicable

If replication factor `-1` is used (implying to use the broker default) the
SandboxKafkaTopicClient created a mocked topic with replication factor 0
instead of using the broker default.

If multiple statement are submitted as once (ie, via a file), later
statement could inherit the incorrect replication factor and thus fail,
which failes all statements in the file.

Closes #4800
@mjsax mjsax requested a review from a team as a code owner December 23, 2021 01:05
@mjsax
Copy link
Member Author

mjsax commented Dec 23, 2021

This fix should be included in 0.24.0 and 7.1.x. I opened the PR against master as it seems 7.1.x branch is broker atm. We would need to cherry-pick this this into 7.1.x.

Copy link
Contributor

@vcrfxia vcrfxia left a comment

Choose a reason for hiding this comment

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

Thanks @mjsax -- LGTM!

@swist
Copy link
Member

swist commented Jan 4, 2022

@mjsax Build is red :( can you double check? Once it's in, can you ping me and I'll cherry pick stuff to 7.1.x

@mjsax
Copy link
Member Author

mjsax commented Jan 5, 2022

This PR broke 3 tests.

It's a little tricky to fix: For a proper fix, we need a change in AK. I opened a PR for the AK change, but it will take some time to get it merged: apache/kafka#11648 (could not find a good workaround within ksqlDB code base).

As an intermediate solution, we could disable the three failing test? Thoughts?

@vcrfxia
Copy link
Contributor

vcrfxia commented Jan 5, 2022

Hey @mjsax how come it's only the INSERT INTO tests in RecoveryTest that are failing? I'm fine with the overall approach of disabling the tests until the fix propagates, as long as we don't lose track of the ticket to reenable them, but I'm curious why it's only these tests that need the fix and not others.

@mjsax
Copy link
Member Author

mjsax commented Jan 5, 2022

Thanks for asking. I never thought about it.

Looking into more, the reason is that those tests use non-existing topic B. I updated the PR to re-create topic B which fixes the tests.

@mjsax mjsax merged commit be838e7 into confluentinc:master Jan 6, 2022
@mjsax mjsax deleted the gh4800-replication-factor branch January 6, 2022 22:12
mjsax added a commit that referenced this pull request Jan 6, 2022
… applicable (#8551)

If replication factor `-1` is used (implying to use the broker default) the SandboxKafkaTopicClient created a mocked topic with replication factor 0 instead of using the broker default.

If multiple statement are submitted as once (ie, via a file), later statements inherit the incorrect replication factor of 0 and thus fail, which fails all statements in the file.

Fix PR fixes SandboxKafkaTopicClient to use the correct default replication factor and thus allow to submit multiple dependent statement at once.
@mjsax
Copy link
Member Author

mjsax commented Jan 6, 2022

Merged to master and (blind) cherry-picked to 7.1.x (as 7.1.x is broken atm).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants