Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2054058198 failed tests pass on my local. will merge it. ``` ./gradlew cleanTest :streams:test --tests SlidingWindowedKStreamIntegrationTest.shouldRestoreAfterJoinRestart --tests StreamsAssignmentScaleTest.testHighAvailabilityTaskAssignorLargeNumConsumers :tools:test --tests MetadataQuorumCommandTest.testDescribeQuorumReplicationSuccessful --tests MetadataQuorumCommandTest.testDescribeQuorumStatusSuccessful --tests ReassignPartitionsIntegrationTest.testProduceAndConsumeWithReassignmentInProgress --tests ReassignPartitionsIntegrationTest.testReassignment --tests ReassignPartitionsIntegrationTest.testHighWaterMarkAfterPartitionReassignment :storage:test --tests TransactionsWithTieredStoreTest.testAbortTransactionTimeout :connect:runtime:test --tests org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testAddingWorker --tests org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testRemovingWorker :trogdor:test --tests CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated :connect:mirror:tes t --tests MirrorConnectorsIntegrationSSLTest.testSyncTopicConfigs :core:test --tests DelegationTokenEndToEndAuthorizationWithOwnerTest.testProduceConsumeViaSubscribe --tests DelegationTokenEndToEndAuthorizationWithOwnerTest.testCreateUserWithDelegationToken --tests ConsumerBounceTest.testConsumptionWithBrokerFailures --tests ConsumerBounceTest.testSeekAndCommitWithBrokerFailures --tests PlaintextConsumerTest.testCoordinatorFailover --tests SaslMultiMechanismConsumerTest.testCoordinatorFailover --tests SslConsumerTest.testCoordinatorFailover ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2054058383 @Owen-CH-Leung thanks for your contribution and effort! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 merged PR #15489: URL: https://github.com/apache/kafka/pull/15489 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1564488318
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -48,7 +53,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
Review Comment:
Thanks! I've pushed a commit to address all your comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1564147419
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -48,7 +53,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
Review Comment:
> ClusterTestDefaults didn't have the API serverProperties so I think it
wouldn't compile
It is supported now. see #15687
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1564142762
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -48,7 +53,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
Review Comment:
those `serverProperties` are annotated with `@BeforeEach`. Do you mean you
want to put them like below ?
```
@ClusterTest(clusterType = Type.ALL, serverProperties = {
@ClusterConfigProperty(key = "auto.create.topics.enable", value
= "false"),
@ClusterConfigProperty(key = "offsets.topic.replication.factor",
value = "1"),
@ClusterConfigProperty(key = "offsets.topic.num.partitions",
value = "4")
})
```
If so we'd have to modify each test. `ClusterTestDefaults` didn't have the
API `serverProperties` so I think it wouldn't compile
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1564142762
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -48,7 +53,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
Review Comment:
those server config are annotated with `@BeforeEach`. Do you mean you want
to put them like below ?
```
@ClusterTest(clusterType = Type.ALL, serverProperties = {
@ClusterConfigProperty(key = "auto.create.topics.enable", value
= "false"),
@ClusterConfigProperty(key = "offsets.topic.replication.factor",
value = "1"),
@ClusterConfigProperty(key = "offsets.topic.num.partitions",
value = "4")
})
```
If so we'd have to modify each test. `ClusterTestDefaults` didn't have the
API `serverProperties` so I think it wouldn't compile
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1564099443
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -48,7 +53,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
public class GetOffsetShellTest {
private final int topicCount = 4;
Review Comment:
`topicName` could be a local variable
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -81,28 +86,57 @@ private void setUp() {
}
Properties props = new Properties();
-props.put(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG,
cluster.config().producerProperties().get("bootstrap.servers"));
+props.put(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG,
cluster.bootstrapServers());
props.put(ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG,
StringSerializer.class);
props.put(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG,
StringSerializer.class);
try (KafkaProducer producer = new
KafkaProducer<>(props)) {
IntStream.range(0, topicCount + 1)
.forEach(i -> IntStream.range(0, i * i)
-.forEach(msgCount -> producer.send(
-new ProducerRecord<>(getTopicName(i), msgCount
% i, null, "val" + msgCount)))
+.forEach(msgCount -> {
+try {
Review Comment:
we can leverage `assertDoesNotThrow` to simplify the code
```java
assertDoesNotThrow(() -> producer.send(
new ProducerRecord<>(getTopicName(i),
msgCount % i, null, "val" + msgCount)).get());
```
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -48,7 +53,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
Review Comment:
Could you use `serverProperties` to define default server configs?
```java
cluster.config().serverProperties().put("auto.create.topics.enable",
false);
cluster.config().serverProperties().put("offsets.topic.replication.factor",
"1");
cluster.config().serverProperties().put("offsets.topic.num.partitions",
String.valueOf(offsetTopicPartitionCount));
```
could be replaced by
```java
@ClusterTestDefaults(clusterType = Type.ALL, serverProperties = {
@ClusterConfigProperty(key = "auto.create.topics.enable", value =
"false"),
@ClusterConfigProperty(key = "offsets.topic.replication.factor", value =
"1"),
@ClusterConfigProperty(key = "offsets.topic.num.partitions", value = "4")
})
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2053607875 > @Owen-CH-Leung > > The root cause is that not all produce records succeed to be sent, and we don't check all sends before closing producer. As our CI is very busy, it could case timeout if we send many records at once. Hence, could you call `get` on the `Producer#send` to make sure those records get sent. Thanks a lot for helping to identify the root cause! I've just pushed a commit to address all your comments. Let's see what we got from the build =) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1562683822
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +428,30 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, Supplier>
outputSupplier) {
+try {
+TestUtils.waitForCondition(
+() -> expected.equals(outputSupplier.get()),
+"TopicOffsets did not match. Expected: " +
expectedTestTopicOffsets() + ", but was: "
++ outputSupplier.get() + ". Final offsets: " +
getFinalOffsets()
+);
+} catch (InterruptedException | ExecutionException e) {
+throw new RuntimeException(e);
+}
+}
+
+private Set getFinalOffsets() throws ExecutionException,
InterruptedException {
Review Comment:
we can remove this function now.
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -94,15 +109,38 @@ private void setUp() {
}
}
+private void createConsumerAndPoll() {
+Properties props = new Properties();
+props.put(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG,
cluster.bootstrapServers());
+props.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG,
StringDeserializer.class);
+props.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG,
StringDeserializer.class);
+props.put(ConsumerConfig.GROUP_ID_CONFIG, "test-consumer-group");
+props.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
+
+try (KafkaConsumer consumer = new
KafkaConsumer<>(props)) {
+List topics = new ArrayList<>();
+for (int i = 0; i < topicCount + 1; i++) {
+topics.add(getTopicName(i));
+}
+consumer.subscribe(topics);
+consumer.poll(consumerTimeout);
+}
+}
+
static class Row {
private String name;
private int partition;
-private Long timestamp;
+private Long offset;
Review Comment:
Could you add `final` to those fields?
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +428,30 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, Supplier>
outputSupplier) {
+try {
+TestUtils.waitForCondition(
Review Comment:
we don't need this retry now
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -48,13 +62,14 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
public class GetOffsetShellTest {
private final int topicCount = 4;
private final int offsetTopicPartitionCount = 4;
private final ClusterInstance cluster;
private final String topicName = "topic";
+private final Duration consumerTimeout = Duration.ofMillis(1000);
Review Comment:
this can be a local variable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2042555906 > Look like the build still contains failed test :( yep, I have filed another #15654 to dig in that :_ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2042547986 > rebase to trigger QA again Look like the build still contains failed test :( -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2041136463 rebase to trigger QA again -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2038933841 blocked by #15663 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1550273635
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +420,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, Supplier>
outputSupplier) {
+try {
+TestUtils.waitForCondition(
+() -> expected.equals(outputSupplier.get()),
+"TopicOffsets did not match. Expected: " +
expectedTestTopicOffsets() + ", but was: " + outputSupplier.get()
Review Comment:
I have the similar error in another PR
(https://github.com/apache/kafka/pull/15621) :(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1550273635
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +420,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, Supplier>
outputSupplier) {
+try {
+TestUtils.waitForCondition(
+() -> expected.equals(outputSupplier.get()),
+"TopicOffsets did not match. Expected: " +
expectedTestTopicOffsets() + ", but was: " + outputSupplier.get()
Review Comment:
I have the same error in another PR
(https://github.com/apache/kafka/pull/15621) :(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1548103258
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +420,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, Supplier>
outputSupplier) {
+try {
+TestUtils.waitForCondition(
+() -> expected.equals(outputSupplier.get()),
+"TopicOffsets did not match. Expected: " +
expectedTestTopicOffsets() + ", but was: " + outputSupplier.get()
Review Comment:
Ok done. Let's see what we get from there =)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1548067397
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +420,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, Supplier>
outputSupplier) {
+try {
+TestUtils.waitForCondition(
+() -> expected.equals(outputSupplier.get()),
+"TopicOffsets did not match. Expected: " +
expectedTestTopicOffsets() + ", but was: " + outputSupplier.get()
Review Comment:
yep, I loop the test with this PR 1000 times, all pass :(
It seems we need to reproduce the failure on our CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1548061572
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +420,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, Supplier>
outputSupplier) {
+try {
+TestUtils.waitForCondition(
+() -> expected.equals(outputSupplier.get()),
+"TopicOffsets did not match. Expected: " +
expectedTestTopicOffsets() + ", but was: " + outputSupplier.get()
Review Comment:
So sth like below ? Print out the final offset as error msg for debugging ?
```
TestUtils.waitForCondition(
() -> expected.equals(outputSupplier.get()),
"TopicOffsets did not match. Expected: " +
expectedTestTopicOffsets() + ", but was: "
+ outputSupplier.get() + ". Final offsets: " +
getFinalOffsets()
);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1547986293
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +420,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, Supplier>
outputSupplier) {
+try {
+TestUtils.waitForCondition(
+() -> expected.equals(outputSupplier.get()),
+"TopicOffsets did not match. Expected: " +
expectedTestTopicOffsets() + ", but was: " + outputSupplier.get()
Review Comment:
Could you use `admin` to list latest offsets after it fails? for example:
```java
private Set parse() throws ExecutionException, InterruptedException
{
try (Admin admin = cluster.createAdminClient()) {
Set topics = admin.listTopics(new
ListTopicsOptions().listInternal(true)).listings().get()
.stream().map(TopicListing::name).collect(Collectors.toSet());
Map offsetRequest =
admin.describeTopics(topics)
.allTopicNames().get().entrySet().stream().flatMap(entry
-> entry.getValue().partitions()
.stream().map(p -> new
AbstractMap.SimpleImmutableEntry<>(new TopicPartition(entry.getKey(),
p.partition()), OffsetSpec.latest(
.collect(Collectors.toMap(Map.Entry::getKey,
Map.Entry::getValue));
return
admin.listOffsets(offsetRequest).all().get().entrySet().stream()
.map(entry -> new Row(entry.getKey().topic(),
entry.getKey().partition(), entry.getValue().offset()))
.collect(Collectors.toSet());
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1547934887
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +447,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, List output) {
Review Comment:
> The build still contains falied test even when retry is implemented...
looks like it's due to race condition when executing the tests ?
all failed tests are running in kraft mode, so I agree up to a point...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1547789497
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +447,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, List output) {
Review Comment:
@chia7712 The build still contains falied test even when retry is
implemented... looks like it's due to race condition when executing the tests ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1547302390
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +447,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, List output) {
Review Comment:
Ahh yes. I've revised to accept a function now. The output should be
refreshed during the wait loop. Let's see how the build goes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1547303323
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -94,15 +101,47 @@ private void setUp() {
}
}
+private void createConsumerAndPoll() {
+Properties props = new Properties();
+props.put(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG,
cluster.bootstrapServers());
+props.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG,
StringDeserializer.class);
+props.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG,
StringDeserializer.class);
+props.put(ConsumerConfig.GROUP_ID_CONFIG, "test-consumer-group");
+props.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
+
+try (KafkaConsumer consumer = new
KafkaConsumer<>(props)) {
+List topics = new ArrayList<>();
+for (int i = 0; i < topicCount + 1; i++) {
+topics.add(getTopicName(i));
+}
+consumer.subscribe(topics);
+consumer.poll(consumerTimeout);
+TestUtils.waitForCondition(
Review Comment:
Yess. Removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1547302390
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +447,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, List output) {
Review Comment:
Ahh yes. I've revised to accept a function now. The output should be
refreshed during the wait loop
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1546808904
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -94,15 +101,47 @@ private void setUp() {
}
}
+private void createConsumerAndPoll() {
+Properties props = new Properties();
+props.put(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG,
cluster.bootstrapServers());
+props.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG,
StringDeserializer.class);
+props.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG,
StringDeserializer.class);
+props.put(ConsumerConfig.GROUP_ID_CONFIG, "test-consumer-group");
+props.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
+
+try (KafkaConsumer consumer = new
KafkaConsumer<>(props)) {
+List topics = new ArrayList<>();
+for (int i = 0; i < topicCount + 1; i++) {
+topics.add(getTopicName(i));
+}
+consumer.subscribe(topics);
+consumer.poll(consumerTimeout);
+TestUtils.waitForCondition(
Review Comment:
we can get rid of this wait
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1546808666
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -391,4 +447,15 @@ private String[] addBootstrapServer(String... args) {
return newArgs.toArray(new String[0]);
}
+
+private void retryUntilEqual(List expected, List output) {
Review Comment:
the `output` should be a function. Otherwise, the output always returns same
value in the waiting loop
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2029635510 > > Shall we just add the retry logic for tests that perform assertion based on the Row class ? > > yep, `waitForCondition` can address that for you :) Ok. I've implemented retry logic using `waitForCondition`. Let's see how the build goes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2029493994 > Shall we just add the retry logic for tests that perform assertion based on the Row class ? yep, `waitForCondition` can address that for you :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2029487529 > assertEquals(expectedTestTopicOffsets().stream().filter(r -> r.partition <= 1).collect(Collectors.toList()), offsets); Oh got it. I read through the previous CI failure and notice that some tests like `testInternalExcluded`, `testNoFilterOptions` are intermittently failing also. Shall we just add the retry logic for tests that perform assertion based on the Row class ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2028866594 > Sure. I've used waitForCondition to wait for __consumer_offsets to be created when creating consumers to poll I meant `assertEquals(expectedTestTopicOffsets().stream().filter(r -> r.partition <= 1).collect(Collectors.toList()), offsets);` should be retried since the metadata is not updated. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2028580025 > @Owen-CH-Leung Could you use `TestUtils.waitForCondition` to verify the records? maybe our QA is too slow to update the metadata before we do the check Sure. I've used `waitForCondition` to wait for `__consumer_offsets` to be created when creating consumers to poll -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
mcmmining commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2028548626 > > printUsageAndExit > > Agree. Setting a dumb exit procedure solves the failed build. Let me revise that -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2028541632 @Owen-CH-Leung Could you use `TestUtils.waitForCondition` to verify the records? maybe our QA is too slow to update the metadata before we do the check -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2028041612 rebase code to include the fix https://github.com/apache/kafka/commit/9a9b532d5d5beeecf7a4b769731ee609625429e1 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1545199583
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -104,6 +104,11 @@ public Row(String name, int partition, Long timestamp) {
this.timestamp = timestamp;
}
+@Override
+public String toString() {
+return "Row[name:" + name + ",partition:" + partition +
",timestamp:" + timestamp;
Review Comment:
Thanks. Amended to use offset instead of timestamp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1545074098
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -104,6 +104,11 @@ public Row(String name, int partition, Long timestamp) {
this.timestamp = timestamp;
}
+@Override
+public String toString() {
+return "Row[name:" + name + ",partition:" + partition +
",timestamp:" + timestamp;
Review Comment:
https://github.com/apache/kafka/blob/trunk/tools/src/main/java/org/apache/kafka/tools/GetOffsetShell.java#L93
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1545058512
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -104,6 +104,11 @@ public Row(String name, int partition, Long timestamp) {
this.timestamp = timestamp;
}
+@Override
+public String toString() {
+return "Row[name:" + name + ",partition:" + partition +
",timestamp:" + timestamp;
Review Comment:
I tried to do the refactoring to use `offset` rather than `timestamp`, but
I'm a bit lost as in how I can get the offset number from the `GetOffsetShell`.
I tried to read through the CI output but couldn't locate where the output is
an offset rather than the timestamp. Could you point me to the outputs that you
saw so that I can get more context of it ? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1544639778
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -104,6 +104,11 @@ public Row(String name, int partition, Long timestamp) {
this.timestamp = timestamp;
}
+@Override
+public String toString() {
+return "Row[name:" + name + ",partition:" + partition +
",timestamp:" + timestamp;
Review Comment:
yep, the output from shell is offset rather than timestamp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1544636515
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -104,6 +104,11 @@ public Row(String name, int partition, Long timestamp) {
this.timestamp = timestamp;
}
+@Override
+public String toString() {
+return "Row[name:" + name + ",partition:" + partition +
",timestamp:" + timestamp;
Review Comment:
you mean like `return "Row[name:" + name + ",partition:" + partition +
",offset:" + offset;` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1544483152
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -104,6 +104,11 @@ public Row(String name, int partition, Long timestamp) {
this.timestamp = timestamp;
}
+@Override
+public String toString() {
+return "Row[name:" + name + ",partition:" + partition +
",timestamp:" + timestamp;
Review Comment:
Could you rename the `timestamp` to `offset`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2026412628 > adde167 No prob. Added back `ToString` to troubleshoot -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2025290627 @Owen-CH-Leung Could you add `toString` back? It seems the test is unstable and we need to dig in it :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1542557448
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -48,13 +52,14 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
public class GetOffsetShellTest {
private final int topicCount = 4;
private final int offsetTopicPartitionCount = 4;
private final ClusterInstance cluster;
private final String topicName = "topic";
+private final Duration consumerTimeout = Duration.ofMillis(100);
Review Comment:
Sure. Bumped the timeout to 1000.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1542532419
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -48,13 +52,14 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
public class GetOffsetShellTest {
private final int topicCount = 4;
private final int offsetTopicPartitionCount = 4;
private final ClusterInstance cluster;
private final String topicName = "topic";
+private final Duration consumerTimeout = Duration.ofMillis(100);
Review Comment:
Could you increase the timeout to run QA again? The failed tests are caused
that `__consumer_offsets` not created.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2024287098 > printUsageAndExit Agree. Setting a dumb exit procedure solves the failed build. Let me revise that -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489:
URL: https://github.com/apache/kafka/pull/15489#issuecomment-2023109755
@Owen-CH-Leung The `printUsageAndExit` call `exit(1)` so `KRAFT` and
`CO_KRAFT` will stop the JVM. `ZK` can capture the exit code to throw exception
so it does not terminate the JVM.
Hence, a simple solution is - set dumb exit procedure for `testPrintHelp`.
For example:
```java
@ClusterTest
public void testPrintHelp() {
Exit.setExitProcedure((statusCode, message) -> { });
try {
String out = ToolsTestUtils.captureStandardErr(() ->
GetOffsetShell.mainNoExit("--help"));
assertTrue(out.startsWith(GetOffsetShell.USAGE_TEXT));
} finally {
Exit.resetExitProcedure();
}
}
```
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2020499827 It seems all builds failed due to `tools` module. Could you check them please? ``` [2024-03-26T11:58:09.997Z] Execution failed for task ':tools:test'. [2024-03-26T11:58:09.997Z] > Process 'Gradle Test Executor 84' finished with non-zero exit value 1 [2024-03-26T11:58:09.997Z] This problem might be caused by incorrect test process configuration. [2024-03-26T11:58:09.997Z] For more on test execution, please refer to https://docs.gradle.org/8.6/userguide/java_testing.html#sec:test_execution in the Gradle documentation. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on PR #15489: URL: https://github.com/apache/kafka/pull/15489#issuecomment-2019613861 @Owen-CH-Leung Could you please rebase code to trigger QA again? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1537117543
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -47,14 +51,16 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
public class GetOffsetShellTest {
private final int topicCount = 4;
private final int offsetTopicPartitionCount = 4;
private final ClusterInstance cluster;
private final String topicName = "topic";
+private final int consumerTimeout = 100;
Review Comment:
Sure - Updated to use `Duration`
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -181,13 +214,17 @@ public void testTopicPatternArgWithPartitionsArg() {
public void testTopicPartitionsArg() {
setUp();
+createConsumerAndPoll();
+
List offsets = executeAndParse("--topic-partitions",
"topic1:0,topic2:1,topic(3|4):2,__.*:3");
-List expected = Arrays.asList(
+List expected = new ArrayList<>(
Review Comment:
Thanks. Reverted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1537094676
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -47,14 +51,16 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
@ExtendWith(value = ClusterTestExtensions.class)
-@ClusterTestDefaults(clusterType = Type.ZK)
+@ClusterTestDefaults(clusterType = Type.ALL)
@Tag("integration")
public class GetOffsetShellTest {
private final int topicCount = 4;
private final int offsetTopicPartitionCount = 4;
private final ClusterInstance cluster;
private final String topicName = "topic";
+private final int consumerTimeout = 100;
Review Comment:
How about using `Duration` here?
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -181,13 +214,17 @@ public void testTopicPatternArgWithPartitionsArg() {
public void testTopicPartitionsArg() {
setUp();
+createConsumerAndPoll();
+
List offsets = executeAndParse("--topic-partitions",
"topic1:0,topic2:1,topic(3|4):2,__.*:3");
-List expected = Arrays.asList(
+List expected = new ArrayList<>(
Review Comment:
This change is unnecessary now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1537089434
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -283,7 +318,7 @@ public void testTopicPartitionsArgWithInternalExcluded() {
assertEquals(expected, offsets);
}
-@ClusterTest
+@ClusterTest(clusterType = Type.ZK)
Review Comment:
Sure - amended the test to create consumer for polling
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -338,6 +373,7 @@ private void assertExitCodeIsOne(String... args) {
}
private List expectedOffsetsWithInternal() {
+
Review Comment:
Sure & reverted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1536623947
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -338,6 +373,7 @@ private void assertExitCodeIsOne(String... args) {
}
private List expectedOffsetsWithInternal() {
+
Review Comment:
could you please revert this unnecessary change?
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -283,7 +318,7 @@ public void testTopicPartitionsArgWithInternalExcluded() {
assertEquals(expected, offsets);
}
-@ClusterTest
+@ClusterTest(clusterType = Type.ZK)
Review Comment:
Could we create consumer to create internal topic for this test case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1534974289
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -182,14 +187,19 @@ public void testTopicPartitionsArg() {
setUp();
List offsets = executeAndParse("--topic-partitions",
"topic1:0,topic2:1,topic(3|4):2,__.*:3");
-List expected = Arrays.asList(
-new Row("__consumer_offsets", 3, 0L),
+ArrayList expected = new ArrayList<>(
+Arrays.asList(
new Row("topic1", 0, 1L),
new Row("topic2", 1, 2L),
new Row("topic3", 2, 3L),
new Row("topic4", 2, 4L)
+)
);
+if (!cluster.isKRaftTest()) {
Review Comment:
Sure. I've created Consumer to subscribe and poll from the topic for this
test.
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -182,14 +187,19 @@ public void testTopicPartitionsArg() {
setUp();
List offsets = executeAndParse("--topic-partitions",
"topic1:0,topic2:1,topic(3|4):2,__.*:3");
-List expected = Arrays.asList(
-new Row("__consumer_offsets", 3, 0L),
+ArrayList expected = new ArrayList<>(
+Arrays.asList(
new Row("topic1", 0, 1L),
new Row("topic2", 1, 2L),
new Row("topic3", 2, 3L),
new Row("topic4", 2, 4L)
+)
);
+if (!cluster.isKRaftTest()) {
Review Comment:
Sure. I've created Consumer to subscribe and poll from the topic for this
test, followed by verifying the pattern
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1534178664
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -182,14 +187,19 @@ public void testTopicPartitionsArg() {
setUp();
List offsets = executeAndParse("--topic-partitions",
"topic1:0,topic2:1,topic(3|4):2,__.*:3");
-List expected = Arrays.asList(
-new Row("__consumer_offsets", 3, 0L),
+ArrayList expected = new ArrayList<>(
+Arrays.asList(
new Row("topic1", 0, 1L),
new Row("topic2", 1, 2L),
new Row("topic3", 2, 3L),
new Row("topic4", 2, 4L)
+)
);
+if (!cluster.isKRaftTest()) {
Review Comment:
> Shall we create an internal topic when running in Kraft mode ?
yep, we can create a consumer to read records before verifying the match
pattern
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1534175100
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -182,14 +187,19 @@ public void testTopicPartitionsArg() {
setUp();
List offsets = executeAndParse("--topic-partitions",
"topic1:0,topic2:1,topic(3|4):2,__.*:3");
-List expected = Arrays.asList(
-new Row("__consumer_offsets", 3, 0L),
+ArrayList expected = new ArrayList<>(
+Arrays.asList(
new Row("topic1", 0, 1L),
new Row("topic2", 1, 2L),
new Row("topic3", 2, 3L),
new Row("topic4", 2, 4L)
+)
);
+if (!cluster.isKRaftTest()) {
Review Comment:
Currently we didn't create any internal topic when running in kraft mode, so
we can't verify the match pattern `__.*:3`. Shall we create an internal topic
when running in Kraft mode ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1534168170
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -338,6 +348,10 @@ private void assertExitCodeIsOne(String... args) {
}
private List expectedOffsetsWithInternal() {
+if (cluster.isKRaftTest()) {
Review Comment:
Sure - I've moved the evaluation of `cluster.isKRaftTest()` to the tests
instead of putting it in the helper API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1534166771
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -182,14 +187,19 @@ public void testTopicPartitionsArg() {
setUp();
List offsets = executeAndParse("--topic-partitions",
"topic1:0,topic2:1,topic(3|4):2,__.*:3");
-List expected = Arrays.asList(
-new Row("__consumer_offsets", 3, 0L),
+ArrayList expected = new ArrayList<>(
Review Comment:
Sure - I've changed to use `List`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1534165905
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -104,6 +104,11 @@ public Row(String name, int partition, Long timestamp) {
this.timestamp = timestamp;
}
+@Override
+public String toString() {
+return "Row[name:" + name + ",partition:" + partition +
",timestamp:" + timestamp;
Review Comment:
Yes you are right. Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1533239354
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -182,14 +187,19 @@ public void testTopicPartitionsArg() {
setUp();
List offsets = executeAndParse("--topic-partitions",
"topic1:0,topic2:1,topic(3|4):2,__.*:3");
-List expected = Arrays.asList(
-new Row("__consumer_offsets", 3, 0L),
+ArrayList expected = new ArrayList<>(
+Arrays.asList(
new Row("topic1", 0, 1L),
new Row("topic2", 1, 2L),
new Row("topic3", 2, 3L),
new Row("topic4", 2, 4L)
+)
);
+if (!cluster.isKRaftTest()) {
Review Comment:
I feel this test needs to verify the match pattern `__.*:3`, so we can do
read for KRaft mode to create the internal topics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Kafka-15729: Add KRaft support in GetOffsetShellTest [kafka]
chia7712 commented on code in PR #15489:
URL: https://github.com/apache/kafka/pull/15489#discussion_r1533209761
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -104,6 +104,11 @@ public Row(String name, int partition, Long timestamp) {
this.timestamp = timestamp;
}
+@Override
+public String toString() {
+return "Row[name:" + name + ",partition:" + partition +
",timestamp:" + timestamp;
Review Comment:
Is it used for debugging?
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -338,6 +348,10 @@ private void assertExitCodeIsOne(String... args) {
}
private List expectedOffsetsWithInternal() {
+if (cluster.isKRaftTest()) {
Review Comment:
I prefer to make callers use `expectedTestTopicOffsets` instead of adding
if-else here.
##
tools/src/test/java/org/apache/kafka/tools/GetOffsetShellTest.java:
##
@@ -182,14 +187,19 @@ public void testTopicPartitionsArg() {
setUp();
List offsets = executeAndParse("--topic-partitions",
"topic1:0,topic2:1,topic(3|4):2,__.*:3");
-List expected = Arrays.asList(
-new Row("__consumer_offsets", 3, 0L),
+ArrayList expected = new ArrayList<>(
Review Comment:
It seems we can keep using `List`, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] Kafka-15729: Add KRaft support in GetOffsetShellTest [kafka]
Owen-CH-Leung opened a new pull request, #15489: URL: https://github.com/apache/kafka/pull/15489 https://issues.apache.org/jira/browse/KAFKA-15729 As per [KAFKA-15729](https://issues.apache.org/jira/browse/KAFKA-15729), this PR adds Kraft support to `GetOffsetShellTest` ### Committer Checklist (excluded from commit message) - [x] Verify design and implementation - [x] Verify test coverage and CI build status - [x] Verify documentation (including upgrade notes) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
