You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "FrankYang0529 (via GitHub)" <gi...@apache.org> on 2024/08/11 07:42:52 UTC
[PR] KAFKA-17309: Fix flaky testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException [kafka]
FrankYang0529 opened a new pull request, #16854:
URL: https://github.com/apache/kafka/pull/16854
When closing `KafkaAdminClient`, the process may hang, because there is active external request. Prepare the response for the request, so the `KafkaAdminClient` can close smoothly.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17309: Fix flaky testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #16854:
URL: https://github.com/apache/kafka/pull/16854#discussion_r1713005936
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -8192,6 +8193,9 @@ public void testListClientMetricsResourcesNotSupported() {
@Test
public void testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException() throws InterruptedException {
try (AdminClientUnitTestEnv env = new AdminClientUnitTestEnv(mockCluster(1, 0))) {
+ // mock AdminMetadataManager for KafkaAdminClient, so closing the default one
Review Comment:
Yes, updated it. Thanks for the review.
--
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] KAFKA-17309: Fix flaky testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #16854:
URL: https://github.com/apache/kafka/pull/16854#issuecomment-2284051053
Hi @chia7712, thanks for the review. I addressed all comments. Failed cases in CI can't reproduce on my laptop.
```
./gradlew cleanTest :streams:test --tests RestoreIntegrationTest.shouldInvokeUserDefinedGlobalStateRestoreListener :trogdor:test --tests CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated :core:test --tests PlaintextAdminIntegrationTest.testDescribeTransactions --tests DelegationTokenRequestsTest.testDelegationTokenRequests --tests ZkMigrationFailoverTest.testDriverSkipsEventsFromOlderEpoch :clients:test --tests MemoryRecordsBuilderTest.testBuffersDereferencedOnClose
```
--
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] KAFKA-17309: Fix flaky testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16854:
URL: https://github.com/apache/kafka/pull/16854#discussion_r1713053084
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -8206,16 +8223,25 @@ public void testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModi
// avoid sending fetchMetadata request
doReturn(1L).when(metadataManager).metadataFetchDelayMs(anyLong());
- env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+ mockCLient.setNodeApiVersions(NodeApiVersions.create());
try (KafkaAdminClient admin = KafkaAdminClient.createInternal(
- new AdminClientConfig(Collections.emptyMap()), metadataManager, env.kafkaClient(), env.time())) {
- admin.describeCluster(new DescribeClusterOptions().timeoutMs(1000));
+ new AdminClientConfig(Collections.emptyMap()), metadataManager, mockCLient, Time.SYSTEM)) {
+ DescribeClusterResult result = admin.describeCluster(new DescribeClusterOptions());
// make sure maybeDrainPendingCalls doesn't remove duplicate pending calls
// the listNodes call will be added again in call.fail and remove one in maybeDrainPendingCalls
- TestUtils.waitForCondition(() -> env.kafkaClient().inFlightRequestCount() != 0,
+ TestUtils.waitForCondition(() -> mockCLient.inFlightRequestCount() != 0,
"Timed out waiting for listNodes request");
+
+ // after handleUnsupportedVersionException, describe cluster use MetadataRequest
+ ClientRequest request = mockCLient.requests().peek();
+ assertNotNull(request.apiKey());
Review Comment:
both line 8239 and line 8240 could be replaced by `assertEquals(ApiKeys.METADATA, request.apiKey());`
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -8206,16 +8223,25 @@ public void testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModi
// avoid sending fetchMetadata request
doReturn(1L).when(metadataManager).metadataFetchDelayMs(anyLong());
- env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+ mockCLient.setNodeApiVersions(NodeApiVersions.create());
try (KafkaAdminClient admin = KafkaAdminClient.createInternal(
- new AdminClientConfig(Collections.emptyMap()), metadataManager, env.kafkaClient(), env.time())) {
- admin.describeCluster(new DescribeClusterOptions().timeoutMs(1000));
+ new AdminClientConfig(Collections.emptyMap()), metadataManager, mockCLient, Time.SYSTEM)) {
+ DescribeClusterResult result = admin.describeCluster(new DescribeClusterOptions());
// make sure maybeDrainPendingCalls doesn't remove duplicate pending calls
// the listNodes call will be added again in call.fail and remove one in maybeDrainPendingCalls
- TestUtils.waitForCondition(() -> env.kafkaClient().inFlightRequestCount() != 0,
+ TestUtils.waitForCondition(() -> mockCLient.inFlightRequestCount() != 0,
"Timed out waiting for listNodes request");
+
+ // after handleUnsupportedVersionException, describe cluster use MetadataRequest
+ ClientRequest request = mockCLient.requests().peek();
+ assertNotNull(request.apiKey());
+ assertEquals(ApiMessageType.METADATA, request.apiKey().messageType);
+
+ // clear active external request
+ mockCLient.respondToRequest(request, prepareMetadataResponse(cluster, Errors.NONE));
+ assertDoesNotThrow(() -> result.clusterId().get());
Review Comment:
`assertEquals(cluster.clusterResource().clusterId(), assertDoesNotThrow(() -> result.clusterId().get()));`
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -8191,7 +8192,23 @@ public void testListClientMetricsResourcesNotSupported() {
@Test
public void testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException() throws InterruptedException {
- try (AdminClientUnitTestEnv env = new AdminClientUnitTestEnv(mockCluster(1, 0))) {
+ Cluster cluster = mockCluster(1, 0);
+ try (MockClient mockCLient = new MockClient(Time.SYSTEM, new MockClient.MockMetadataUpdater() {
Review Comment:
`mockCLient` -> `mockClient`
--
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] KAFKA-17309: Fix flaky testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on PR #16854:
URL: https://github.com/apache/kafka/pull/16854#issuecomment-2282719272
I have fetch this PR and loop in local.
It's very stable now.
JDK Version: 21 (Amazon Corretto)
Command I use:
```bash
I=0; while ./gradlew clients:test --tests KafkaAdminClientTest.testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException --rerun --fail-fast; do (( I=$I+1 )); echo "Completed run: $I"; sleep 1; done
```
--
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] KAFKA-17309: Fix flaky testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16854:
URL: https://github.com/apache/kafka/pull/16854#discussion_r1712966860
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -8192,6 +8193,9 @@ public void testListClientMetricsResourcesNotSupported() {
@Test
public void testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException() throws InterruptedException {
try (AdminClientUnitTestEnv env = new AdminClientUnitTestEnv(mockCluster(1, 0))) {
+ // mock AdminMetadataManager for KafkaAdminClient, so closing the default one
Review Comment:
It seems you don't use the `env` actually. Maybe you can remove the env from this test case and create `MockClient` by yourself.
--
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] KAFKA-17309: Fix flaky testCallFailWithUnsupportedVersionExceptionDoesNotHaveConcurrentModificationException [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #16854:
URL: https://github.com/apache/kafka/pull/16854
--
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