Skip to content

MINOR: fix DynamicBrokerReconfigurationTest#22226

Merged
chia7712 merged 2 commits intoapache:trunkfrom
mjsax:minor-fix-DynamicBrokerReconfigurationTest
May 6, 2026
Merged

MINOR: fix DynamicBrokerReconfigurationTest#22226
chia7712 merged 2 commits intoapache:trunkfrom
mjsax:minor-fix-DynamicBrokerReconfigurationTest

Conversation

@mjsax
Copy link
Copy Markdown
Member

@mjsax mjsax commented May 6, 2026

The test is subject to a race-condition. This PR change async to sync
call to fix it.

Reviewers: Chia-Ping Tsai chia7712@gmail.com

The test is subject to a race-condition. This PR change async to sync call to fix it.
@github-actions github-actions Bot added core Kafka Broker tests Test fixes (including flaky tests) small Small PRs labels May 6, 2026
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented May 6, 2026

Call for review @mingyen066 @chia7712 (follow up to #22110)

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM


private def alterConfigs(servers: Seq[KafkaBroker], adminClient: Admin, props: Properties,
perBrokerConfig: Boolean): Unit = {
alterConfigsAsync(servers, adminClient, props, perBrokerConfig).all.get()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Should we wait for all servers to sync to ensure those race conditions go away?

    props.asScala.foreach { case (k, v) =>
      waitForConfig(k, v)
    }

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented May 6, 2026

This PR introduces some race condition on the test. Not sure why CI build did not hit it yet, but we say it in our internal testing.

BTW, PR #22110 just moved some files, so I'm curious how that introduced these race conditions. Would you mind sharing some of your magic with me?

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented May 6, 2026

It's Confluent internal stuff, but this test started to failing for us, when we incorporating #22110. We could also fix internally only, but as it's race condition in AK code base, better to fix upstream?

@chia7712 chia7712 merged commit 85c7227 into apache:trunk May 6, 2026
20 checks passed
@mjsax mjsax deleted the minor-fix-DynamicBrokerReconfigurationTest branch May 7, 2026 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Kafka Broker small Small PRs tests Test fixes (including flaky tests)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants