Skip to content

KAFKA-17631: Convert SaslApiVersionsRequestTest to kraft#18330

Merged
chia7712 merged 1 commit intoapache:trunkfrom
FrankYang0529:KAFKA-17631
Feb 3, 2025
Merged

KAFKA-17631: Convert SaslApiVersionsRequestTest to kraft#18330
chia7712 merged 1 commit intoapache:trunkfrom
FrankYang0529:KAFKA-17631

Conversation

@FrankYang0529
Copy link
Copy Markdown
Member

@FrankYang0529 FrankYang0529 commented Dec 27, 2024

We disabled SaslApiVersionsRequestTest when implementing KAFKA-17614. Re-enable with a new test framework.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@github-actions github-actions Bot added the core Kafka Broker label Dec 27, 2024
@github-actions github-actions Bot added tests Test fixes (including flaky tests) small Small PRs labels Jan 8, 2025
@FrankYang0529 FrankYang0529 force-pushed the KAFKA-17631 branch 3 times, most recently from a63ee08 to 0ce76b6 Compare January 10, 2025 13:57
@chia7712
Copy link
Copy Markdown
Member

@FrankYang0529 could you please fix the conflicts?

@FrankYang0529
Copy link
Copy Markdown
Member Author

@FrankYang0529 could you please fix the conflicts?

Resolved conflict. Thanks.

@FrankYang0529 FrankYang0529 force-pushed the KAFKA-17631 branch 7 times, most recently from e921346 to 298e21b Compare January 30, 2025 00:49
Signed-off-by: PoAn Yang <payang@apache.org>
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 and one unrelated question


@AfterEach
def closeSasl(): Unit = {
sasl.closeSasl()
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.

I'm considering whether we need to close all login modules within Testkit. However, these modules are primarily used to prevent consumer leaks, and we already have thread detection mechanisms in place. @FrankYang0529 WDYT?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we can keep it as current implementation since we have thread leak detection. We can catch error if there is thread leak.

@chia7712 chia7712 merged commit f6f41dc into apache:trunk Feb 3, 2025
chia7712 pushed a commit that referenced this pull request Feb 3, 2025
Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
@FrankYang0529 FrankYang0529 deleted the KAFKA-17631 branch February 4, 2025 01:09
pdruley pushed a commit to pdruley/kafka that referenced this pull request Feb 12, 2025
Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025
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