Re: [PR] KAFKA-14509: [2/N] Implement server side logic for ConsumerGroupDescribe API [kafka]

2023-12-04 Thread via GitHub


dajac merged PR #14544:
URL: https://github.com/apache/kafka/pull/14544


-- 
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



Re: [PR] KAFKA-14509: [2/N] Implement server side logic for ConsumerGroupDescribe API [kafka]

2023-12-01 Thread via GitHub


riedelmax commented on PR #14544:
URL: https://github.com/apache/kafka/pull/14544#issuecomment-1836279426

   Hey @dajac 
   thanks for the review. I addressed all the points except for the ones we 
intent to do a follow up.


-- 
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



Re: [PR] KAFKA-14509: [2/N] Implement server side logic for ConsumerGroupDescribe API [kafka]

2023-12-01 Thread via GitHub


dajac commented on code in PR #14544:
URL: https://github.com/apache/kafka/pull/14544#discussion_r1411829247


##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java:
##
@@ -547,6 +549,69 @@ public CompletableFuture 
listGroups(
 return future;
 }
 
+/**
+ * See {@link GroupCoordinator#consumerGroupDescribe(RequestContext, 
List)}.
+ */
+@Override
+public 
CompletableFuture> 
consumerGroupDescribe(
+RequestContext context,
+List groupIds
+) {
+if (!isActive.get()) {
+return 
CompletableFuture.completedFuture(ConsumerGroupDescribeRequest.getErrorDescribedGroupList(
+groupIds,
+Errors.COORDINATOR_NOT_AVAILABLE
+));
+}
+
+final 
List>> 
futures =
+new ArrayList<>(groupIds.size());
+final Map> groupsByTopicPartition = new 
HashMap<>();
+groupIds.forEach(groupId -> {
+// For backwards compatibility, we support DescribeGroups for the 
empty group id.
+if (groupId == null) {

Review Comment:
   Actually, we don't support the empty group id for this new API. We should 
use `isGroupIdNotEmpty` here.



##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java:
##
@@ -547,6 +549,69 @@ public CompletableFuture 
listGroups(
 return future;
 }
 
+/**
+ * See {@link GroupCoordinator#consumerGroupDescribe(RequestContext, 
List)}.
+ */
+@Override
+public 
CompletableFuture> 
consumerGroupDescribe(
+RequestContext context,
+List groupIds
+) {
+if (!isActive.get()) {
+return 
CompletableFuture.completedFuture(ConsumerGroupDescribeRequest.getErrorDescribedGroupList(
+groupIds,
+Errors.COORDINATOR_NOT_AVAILABLE
+));
+}
+
+final 
List>> 
futures =
+new ArrayList<>(groupIds.size());
+final Map> groupsByTopicPartition = new 
HashMap<>();
+groupIds.forEach(groupId -> {
+// For backwards compatibility, we support DescribeGroups for the 
empty group id.
+if (groupId == null) {

Review Comment:
   Actually, we don't support the empty group id for this new API. We should 
use `isGroupIdNotEmpty` here and remove the comment.



-- 
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