[GitHub] [kafka] showuon commented on a diff in pull request #12329: KAFKA-14010: AlterPartition request won't retry when receiving retriable error

[GitBox <gi...@apache.org>]

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


##########
core/src/test/scala/unit/kafka/server/AlterPartitionManagerTest.scala:
##########
@@ -178,6 +178,46 @@ class AlterPartitionManagerTest {
     assertEquals(request.data().topics().get(0).partitions().size(), 10)
   }
 
+  @Test
+  def testRetryOnPartitionLevelError(): Unit = {
+    // prepare a partition level retriable error response
+    val alterPartitionRespWithPartitionError = partitionResponse(tp0, Errors.UNKNOWN_SERVER_ERROR)
+    val errorResponse = makeClientResponse(alterPartitionRespWithPartitionError, ApiKeys.ALTER_PARTITION.latestVersion)
+
+    val leaderAndIsr = new LeaderAndIsr(1, 1, List(1,2,3), LeaderRecoveryState.RECOVERED, 10)
+    val callbackCapture = ArgumentCaptor.forClass(classOf[ControllerRequestCompletionHandler])
+
+    val scheduler = new MockScheduler(time)
+    val alterPartitionManager = new DefaultAlterPartitionManager(brokerToController, scheduler, time, brokerId, () => 2, () => IBP_3_2_IV0)
+    alterPartitionManager.start()
+    val future = alterPartitionManager.submit(tp0, leaderAndIsr, 0)
+
+    future.whenComplete { (leaderAndIsrResp, e) =>
+      // When entering callback, the `unsentIsrUpdates` element should be removed for possible retry
+      assertFalse(alterPartitionManager.unsentIsrUpdates.containsKey(tp0.topicPartition))
+      assertNull(leaderAndIsrResp)
+      // When receiving retriable error, re-submit the request
+      assertNotNull(e)
+      assertEquals(classOf[UnknownServerException], e.getClass)

Review Comment:
   Yes, you're right, that's misleading! It's because the future callback will just replace the original exception with any new exception thrown from callback, like in the `whenComplete` [java doc](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/CompletionStage.html#whenComplete(java.util.function.BiConsumer)) said:
   
   >  this method is not designed to translate completion outcomes, so the supplied action should not throw an exception. However, if it does, the following rules apply: ... if this stage completed exceptionally and the supplied action throws an exception, then the returned stage completes exceptionally with this stage's exception.
   
   I've updated the test as your suggestion. Thank 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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org