-
Notifications
You must be signed in to change notification settings - Fork 14.8k
KAFKA-19042: [7/N] Move PlaintextConsumerPollTest to client-integration-tests module #19582
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
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. Leave some minor comments.
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
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.
@m1a2st: Thanks for the patch.
I have a few comments.
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
| ) | ||
| public class PlaintextConsumerPollTest { | ||
|
|
||
| public static final int BROKER_COUNT = 3; |
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.
Use short to eliminate the casting.
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 @ClusterTestDefaults brokers type is int, thus we should set BROKER_COUNT as int
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...gration-tests/src/test/java/org/apache/kafka/clients/consumer/PlaintextConsumerPollTest.java
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...egration-tests/src/test/java/org/apache/kafka/clients/consumer/ConsumerAssignmentPoller.java
Outdated
Show resolved
Hide resolved
...gration-tests/src/test/java/org/apache/kafka/clients/consumer/PlaintextConsumerPollTest.java
Outdated
Show resolved
Hide resolved
...gration-tests/src/test/java/org/apache/kafka/clients/consumer/PlaintextConsumerPollTest.java
Outdated
Show resolved
Hide resolved
|
@m1a2st could you please fix the conflicts? |
# Conflicts: # core/src/test/scala/integration/kafka/api/PlaintextConsumerPollTest.scala
|
|
||
| // continuous poll should eventually fail because there is no offset reset strategy set | ||
| // (fail only when resetting positions after coordinator is known) | ||
| TestUtils.waitForCondition(() -> { |
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.
TestUtils.waitForCondition(() -> {
try {
consumer.poll(Duration.ZERO);
return false;
} catch (NoOffsetForPartitionException e) {
return true;
}
}, "Continuous poll not fail");WDYT?
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.
It's good idea, addressed it.
...gration-tests/src/test/java/org/apache/kafka/clients/consumer/PlaintextConsumerPollTest.java
Show resolved
Hide resolved
# Conflicts: # core/src/test/scala/integration/kafka/api/PlaintextConsumerPollTest.scala
Use Java to rewrite PlaintextConsumerPollTest by new test infra and move
it to client-integration-tests module.
Reviewers: PoAn Yang payang@apache.org, TengYao Chi
kitingiao@gmail.com, Chia-Ping Tsai chia7712@gmail.com