-
Notifications
You must be signed in to change notification settings - Fork 14.6k
KAFKA-19666: Remove old restoration codepath from KafkaStreamsTelemetryIntegrationTest [1/N] #20462
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
KAFKA-19666: Remove old restoration codepath from KafkaStreamsTelemetryIntegrationTest [1/N] #20462
Conversation
@@ -444,7 +444,7 @@ private List<String> getTaskIdsAsStrings(final KafkaStreams streams) { | |||
.toList(); | |||
} | |||
|
|||
private static Stream<Arguments> singleAndMultiTaskParameters() { | |||
private static Stream<Arguments> topologyComplexityAndRebalanceProtocol() { |
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.
Renamed to topologyComplexityAndRebalanceProtocol
as per #19275 (comment)
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 removes the state-updater configuration parameter from telemetry integration tests, simplifying test methods by eliminating the boolean parameter that controlled state updater enablement. The changes align with cleanup efforts for state-updater related integration tests.
- Removed
stateUpdaterEnabled
parameter from test methods and helper functions - Simplified parameterized test methods to focus on topology complexity and rebalance protocol
- Converted one parameterized test to a simple test method with hardcoded classic protocol
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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, but a question for @bbejeck
streamsApplicationProperties.put(StreamsConfig.STATE_DIR_CONFIG, TestUtils.tempDirectory(appId).getPath() + "-ks1"); | ||
streamsApplicationProperties.put(StreamsConfig.CLIENT_ID_CONFIG, appId + "-ks1"); | ||
|
||
|
||
streamsSecondApplicationProperties = props(stateUpdaterEnabled, groupProtocol); | ||
streamsSecondApplicationProperties = props("classic"); |
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.
Interesting - why did we not enable this test for the "streams" protocol? @bbejeck
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.
I see that this was disabled recently in #20147
Clean up
KafkaStreamsTelemetryIntegrationTest.java
Reviewers: Lucas Brutschy [email protected]