-
Notifications
You must be signed in to change notification settings - Fork 14.6k
KAFKA-19666: Remove old restoration codepath from RestoreIntegrationTest [4/N] #20498
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
base: trunk
Are you sure you want to change the base?
Conversation
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 cleans up integration tests related to state-updater by removing the state-updater parameterization from test methods in RestoreIntegrationTest.java
. The changes simplify the test structure by removing the state-updater enabled/disabled testing matrix while maintaining coverage for the new protocol parameter.
- Removed state-updater parameterization from all test methods
- Simplified test method signatures and property configuration
- Cleaned up unused imports related to state-updater configuration
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -161,8 +159,8 @@ public void createTopics(final TestInfo testInfo) throws InterruptedException { | |||
CLUSTER.createTopic(inputStream, 2, 1); | |||
} | |||
|
|||
private Properties props(final boolean stateUpdaterEnabled) { | |||
return props(mkObjectProperties(mkMap(mkEntry(InternalConfig.STATE_UPDATER_ENABLED, stateUpdaterEnabled)))); | |||
private Properties props() { |
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.
[nitpick] The props()
method now creates an empty map which might be confusing. Consider adding a comment explaining that this method creates default properties without additional state-updater configuration, or rename it to defaultProps()
for clarity.
private Properties props() { | |
/** | |
* Creates default properties for a Streams application without additional state-updater configuration. | |
*/ | |
private Properties defaultProps() { |
Copilot uses AI. Check for mistakes.
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 patch.
Overall LGTM, just a small nit.
@@ -537,7 +510,7 @@ public void shouldProcessDataFromStoresWithLoggingDisabled(final boolean stateUp | |||
|
|||
final Topology topology = streamsBuilder.build(); | |||
|
|||
final Properties props = props(stateUpdaterEnabled); | |||
final Properties props = props(); | |||
|
|||
if (useNewProtocol) { |
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.
Nit: I noticed a few repeated snippets like
if (useNewProtocol) {
props.put(StreamsConfig.GROUP_PROTOCOL_CONFIG, GroupProtocol.STREAMS.name());
}
Maybe it could be helpful to add small overloads
private Properties props(final boolean useNewProtocol) {
return props(mkObjectProperties(mkMap()), useNewProtocol);
}
private Properties props(final Properties extraProperties, final boolean useNewProtocol) {
final Properties streamsConfiguration = props(extraProperties);
if (useNewProtocol) {
streamsConfiguration.put(StreamsConfig.GROUP_PROTOCOL_CONFIG, GroupProtocol.STREAMS.name());
}
return streamsConfiguration;
}
Then call sites could just use props(useNewProtocol)
or props(extra, useNewProtocol)
to reduce the repeated if blocks.
Clean up
RestoreIntegrationTest.java
Reviewers: Lucas Brutschy [email protected]