[GitHub] [kafka] showuon commented on a change in pull request #11301: KAFKA-13276: Prefer KafkaFuture in admin Result constructors

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

showuon commented on a change in pull request #11301:
URL: https://github.com/apache/kafka/pull/11301#discussion_r703229535



##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/internals/AdminApiFuture.java
##########
@@ -109,7 +110,7 @@ private void completeExceptionally(K key, Throwable t) {
         }
 
         private KafkaFutureImpl<V> futureOrThrow(K key) {
-            KafkaFutureImpl<V> future = futures.get(key);
+            KafkaFutureImpl<V> future = (KafkaFutureImpl<V>) futures.get(key);

Review comment:
       I understand why we need to cast the `KafkaFuture` (parent) class into `KafkaFutureImpl` (child) class here since there are some methods only existed (or public) in child. But I think it'd better to add comment here to explain that this is a safe cast because the future is always `KafkaFutureImpl` instances. If this assumption changed, this should be updated. WDYT?

##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/DeleteConsumerGroupsResult.java
##########
@@ -32,9 +31,9 @@
  */
 @InterfaceStability.Evolving
 public class DeleteConsumerGroupsResult {
-    private final Map<CoordinatorKey, KafkaFutureImpl<Void>> futures;
+    private final Map<CoordinatorKey, KafkaFuture<Void>> futures;
 
-    DeleteConsumerGroupsResult(final Map<CoordinatorKey, KafkaFutureImpl<Void>> futures) {
+    DeleteConsumerGroupsResult(final Map<CoordinatorKey, KafkaFuture<Void>> futures) {

Review comment:
       + 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