You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "wernerdv (via GitHub)" <gi...@apache.org> on 2025/05/04 12:47:02 UTC
[PR] MINOR: Small cleanups in clients tests [kafka]
wernerdv opened a new pull request, #19634:
URL: https://github.com/apache/kafka/pull/19634
Removed unused fields and methods in clients 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Small cleanups in clients tests [kafka]
Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on PR #19634:
URL: https://github.com/apache/kafka/pull/19634#issuecomment-2849202838
@AndrewJSchofield @chia7712 Hello, please take a look.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Small cleanups in clients tests [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #19634:
URL: https://github.com/apache/kafka/pull/19634#discussion_r2073335827
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinatorTest.java:
##########
@@ -1306,7 +1304,7 @@ public void testForceMetadataDeleteForPatternSubscriptionDuringRebalance() {
client.prepareResponse(groupCoordinatorResponse(node, Errors.NONE));
coordinator.ensureCoordinatorReady(time.timer(Long.MAX_VALUE));
- MetadataResponse deletedMetadataResponse = RequestTestUtils.metadataUpdateWith(1, new HashMap<String, Integer>() {
+ MetadataResponse deletedMetadataResponse = RequestTestUtils.metadataUpdateWith(1, new HashMap<>() {
Review Comment:
```java
Map.of(topic1, 1)
```
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Small cleanups in clients tests [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #19634:
URL: https://github.com/apache/kafka/pull/19634#discussion_r2074803762
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -2292,7 +2292,7 @@ private static DescribeLogDirsResponse prepareDescribeLogDirsResponse(Errors err
private static DescribeLogDirsResponse prepareEmptyDescribeLogDirsResponse(Optional<Errors> error) {
DescribeLogDirsResponseData data = new DescribeLogDirsResponseData();
- if (error.isPresent()) data.setErrorCode(error.get().code());
+ error.ifPresent(e -> data.setErrorCode(e.code()));
return new DescribeLogDirsResponse(data);
}
Review Comment:
L506 has an assertion in the wrong order.
Please fix it.
##########
clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java:
##########
@@ -1172,8 +1172,7 @@ public synchronized ListPartitionReassignmentsResult listPartitionReassignments(
Optional<Set<TopicPartition>> partitions,
Review Comment:
L1122
`future.complete(Objects.requireNonNullElseGet(info, () -> new ReplicaLogDirInfo(currentLogDir, 0, null, 0)));`
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Small cleanups in clients tests [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #19634:
URL: https://github.com/apache/kafka/pull/19634
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Small cleanups in clients tests [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #19634:
URL: https://github.com/apache/kafka/pull/19634#discussion_r2073436146
##########
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##########
@@ -570,17 +570,6 @@ public static ByteBuffer toBuffer(UnalignedRecords records) {
return toBuffer(records.toSend());
}
- public static Set<TopicPartition> generateRandomTopicPartitions(int numTopic, int numPartitionPerTopic) {
- Set<TopicPartition> tps = new HashSet<>();
- for (int i = 0; i < numTopic; i++) {
- String topic = randomString(32);
- for (int j = 0; j < numPartitionPerTopic; j++) {
- tps.add(new TopicPartition(topic, j));
- }
- }
- return tps;
- }
-
/**
* Assert that a future raises an expected exception cause type.
* This method will wait for the future to complete or timeout(15000 milliseconds).
Review Comment:
Could you also fix L508 assertion expected and actual position
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Small cleanups in clients tests [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #19634:
URL: https://github.com/apache/kafka/pull/19634#discussion_r2072688367
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java:
##########
@@ -1185,14 +1174,14 @@ public void testNoInterceptorCommitAsyncFailed() {
@Test
public void testRefreshCommittedOffsetsShouldNotResetIfFailedWithTimeout() {
consumer = newConsumer();
- testUpdateFetchPositionsWithFetchCommittedOffsetsTimeout(true);
+ testUpdateFetchPositionsWithFetchCommittedOffsetsTimeout();
}
@Test
public void testRefreshCommittedOffsetsNotCalledIfNoGroupId() {
// Create consumer without group id so committed offsets are not used for updating positions
consumer = newConsumerWithoutGroupId();
- testUpdateFetchPositionsWithFetchCommittedOffsetsTimeout(false);
+ testUpdateFetchPositionsWithFetchCommittedOffsetsTimeout();
Review Comment:
I'm not convinced by this test, or the previous one, at all. It looks like they were trying to test something different, but actually do not. Please can you dig into this a little deeper?
##########
clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java:
##########
@@ -507,7 +507,7 @@ private Map<String, KafkaFuture<TopicDescription>> handleDescribeTopicsByNames(C
return topicDescriptions;
}
- public synchronized Map<Uuid, KafkaFuture<TopicDescription>> handleDescribeTopicsUsingIds(Collection<Uuid> topicIds, DescribeTopicsOptions options) {
+ public synchronized Map<Uuid, KafkaFuture<TopicDescription>> handleDescribeTopicsUsingIds(Collection<Uuid> topicIds) {
Review Comment:
nit: Extra space
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Small cleanups in clients tests [kafka]
Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #19634:
URL: https://github.com/apache/kafka/pull/19634#discussion_r2072915155
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java:
##########
@@ -1185,14 +1174,14 @@ public void testNoInterceptorCommitAsyncFailed() {
@Test
public void testRefreshCommittedOffsetsShouldNotResetIfFailedWithTimeout() {
consumer = newConsumer();
- testUpdateFetchPositionsWithFetchCommittedOffsetsTimeout(true);
+ testUpdateFetchPositionsWithFetchCommittedOffsetsTimeout();
}
@Test
public void testRefreshCommittedOffsetsNotCalledIfNoGroupId() {
// Create consumer without group id so committed offsets are not used for updating positions
consumer = newConsumerWithoutGroupId();
- testUpdateFetchPositionsWithFetchCommittedOffsetsTimeout(false);
+ testUpdateFetchPositionsWithFetchCommittedOffsetsTimeout();
Review Comment:
The original logic that was associated with the method argument was removed in [commit](https://github.com/apache/kafka/commit/6744a718c2c177c7d462b231ea5e476d98f6eb38#diff-c588f5a3c612b1e655eccd370fd43085dd62c6886c1e7340c54123fa2fc55b47L1892) .
It seems that after adding `CheckAndUpdatePositionsEvent` and refactoring the `testUpdateFetchPositionsWithFetchCommittedOffsetsTimeout` method, the `committedOffsetsEnabled` argument is no longer needed.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Small cleanups in clients tests [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #19634:
URL: https://github.com/apache/kafka/pull/19634#issuecomment-2851883929
I will merge it tomorrow if no objection.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Small cleanups in clients tests [kafka]
Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #19634:
URL: https://github.com/apache/kafka/pull/19634#discussion_r2074835327
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -2292,7 +2292,7 @@ private static DescribeLogDirsResponse prepareDescribeLogDirsResponse(Errors err
private static DescribeLogDirsResponse prepareEmptyDescribeLogDirsResponse(Optional<Errors> error) {
DescribeLogDirsResponseData data = new DescribeLogDirsResponseData();
- if (error.isPresent()) data.setErrorCode(error.get().code());
+ error.ifPresent(e -> data.setErrorCode(e.code()));
return new DescribeLogDirsResponse(data);
}
Review Comment:
Fixed
##########
clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java:
##########
@@ -1172,8 +1172,7 @@ public synchronized ListPartitionReassignmentsResult listPartitionReassignments(
Optional<Set<TopicPartition>> partitions,
Review Comment:
Fixed
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org