[GitHub] [kafka] showuon commented on a diff in pull request #13760: KAFKA-8982: Add retry of fetching metadata to Admin.deleteRecords

[via GitHub]

showuon commented on code in PR #13760:
URL: https://github.com/apache/kafka/pull/13760#discussion_r1222617381


##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -2291,6 +2289,8 @@ public void testDeleteRecords() throws Exception {
         try (AdminClientUnitTestEnv env = new AdminClientUnitTestEnv(cluster)) 
{
             env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
 
+            env.kafkaClient().prepareResponse(prepareMetadataResponse(cluster, 
Errors.NONE));

Review Comment:
   Before looking into the implementation, I don't think this test is testing 
what we expected. The original comment is commenting around this lines:
   
https://github.com/apache/kafka/pull/7296/files#diff-5422d10d9a7f4776c6538ae3aea27f24e94cf4ecf5e752040125aca6edc795d3R3671-R3682
   
   And it said, when metadata response (mr) contains error, we just fail the 
future without retry. 
   
   The point is, we want to retry `metadataResponse`, not 
`deleteRecordResponse`. What I expected is tests like this:
   
   
https://github.com/apache/kafka/blob/513e1c641d63c5e15144f9fcdafa1b56c5e5ba09/clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java#L5431C11-L5433
   
   Please update the test and make sure it passed. 
   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