-
Notifications
You must be signed in to change notification settings - Fork 14.5k
KAFKA-19466: LogConcurrencyTest should close the log when the test completes; migrate LogConcurrencyTest #20110
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?
KAFKA-19466: LogConcurrencyTest should close the log when the test completes; migrate LogConcurrencyTest #20110
Conversation
…mpletes; migrate LogConcurrencyTest
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.
@Yunyung thanks for this migration
|
||
validateConsumedData(log, consumer.getConsumedBatches()); | ||
} finally { | ||
executor.shutdownNow(); |
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.
should we wait for all threads to terminate?
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.
Yes! Thanks.
storage/src/test/java/org/apache/kafka/storage/internals/log/LogConcurrencyTest.java
Outdated
Show resolved
Hide resolved
storage/src/test/java/org/apache/kafka/storage/internals/log/LogConcurrencyTest.java
Outdated
Show resolved
Hide resolved
storage/src/test/java/org/apache/kafka/storage/internals/log/LogConcurrencyTest.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Override | ||
public Void call() throws Exception { |
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 method getConsumedBatches
could be removed if call
returns List<FetchedBatch>
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.
Nice.
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.
@Yunyung thanks for this patch
|
||
@Test | ||
public void testUncommittedDataNotConsumed() throws Exception { | ||
testUncommittedDataNotConsumed(createLog()); |
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.
Does the log get closed?
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.
Yes, log is closed in the teardown method.
storage/src/test/java/org/apache/kafka/storage/internals/log/LogConcurrencyTest.java
Show resolved
Hide resolved
); | ||
} | ||
|
||
private record FetchedBatch(long baseOffset, int epoch) { |
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.
Perhaps it could be replaced by LinkedHashMap
?
testUncommittedDataNotConsumed(), which call createLog() but never close
the log when the tests complete.
Jira: https://issues.apache.org/jira/browse/KAFKA-19466 Reviewers:
Chia-Ping Tsai chia7712@gmail.com