-
Notifications
You must be signed in to change notification settings - Fork 14.6k
KAFKA-19550: Integration test for Streams-related Admin APIs [1/N] #20244
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
Conversation
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
Thanks, @lucliu1108, for the PR. I added a couple of comments/suggestions. |
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! This is mostly looking good to me. I left two comments. The main thing is that I would like to avoid the extra dependency from core -> streams. I think it may be possible, following what I did in AuthorizerIntegrationTest
.
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few more minor comments. But this is mostly LGTM
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds integration tests for Streams-related Admin APIs, specifically for describeStreamsGroups
and deleteStreamsGroups
functionality. The changes enable testing of Kafka Streams group management through the Admin client.
Key changes:
- Added
createStreamsGroup
helper method to create test Streams groups with proper configuration - Enhanced existing group listing tests to include Streams groups
- Added comprehensive integration tests for describing and deleting Streams groups
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
PlaintextAdminIntegrationTest.scala | Added two new test methods and updated existing group listing test to handle Streams groups |
IntegrationTestHarness.scala | Added helper method to create Streams groups for testing purposes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still some whitespace changes
core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@@ -2589,8 +2594,11 @@ class PlaintextAdminIntegrationTest extends BaseAdminIntegrationTest { | |||
shareGroupConfig.put(ConsumerConfig.GROUP_ID_CONFIG, shareGroupId) | |||
val shareGroup = createShareConsumer(configOverrides = shareGroupConfig) | |||
|
|||
val config = createConfig | |||
client = Admin.create(config) | |||
val streamsGroup = createStreamsGroup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consumer may create testTopicName
due to a metadata update, causing the subsequent client.createTopics
call to fail since the topic already exists.
see https://github.com/apache/kafka/actions/runs/17495296037/job/49695550299?pr=20472
@lucliu1108 WDYT? Perhaps we could create the topic first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Thanks for pointing out! I'll make a patch for that @chia7712
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change adds:
Admin#describeStreamsGroups
APIAdmin#deleteStreamsGroup
APIReviewers: Alieh Saeedi [email protected], Lucas Brutschy
[email protected]