AndrewJSchofield commented on code in PR #19363:
URL: https://github.com/apache/kafka/pull/19363#discussion_r2035285341
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorRecordHelpers.java:
##########
@@ -814,6 +814,19 @@ public static CoordinatorRecord
newShareGroupCurrentAssignmentTombstoneRecord(
);
}
+ /**
+ * Creates a ShareGroupStatePartitionMetadata tombstone.
+ *
+ * @param groupId The share group id.
+ * @return The record.
+ */
+ public static CoordinatorRecord
newShareGroupStatePartitionMetadataTombstoneRecord(String groupId) {
Review Comment:
nit: Every other method in this file puts the parameter on the next line.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorShard.java:
##########
@@ -579,15 +579,16 @@ public
CoordinatorResult<DeleteGroupsResponseData.DeletableGroupResultCollection
* <p></p>
* The groupIds are first filtered by type to restrict the list to share
groups.
* @param groupIds - A list of groupIds as string
- * @return {@link CoordinatorResult} object always containing empty
records and Map keyed on groupId and value pair (req, error)
+ * @return Map keyed on groupId and value pair (req, error)
Review Comment:
nit: Usually, the style here is "A Result containing .....".
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java:
##########
@@ -1067,25 +1067,27 @@ public
CompletableFuture<DeleteGroupsResponseData.DeletableGroupResultCollection
});
groupsByTopicPartition.forEach((topicPartition, groupList) -> {
-
CompletableFuture<DeleteGroupsResponseData.DeletableGroupResultCollection>
future = deleteShareGroups(topicPartition, groupList).thenCompose(groupErrMap
-> {
- DeleteGroupsResponseData.DeletableGroupResultCollection
collection = new DeleteGroupsResponseData.DeletableGroupResultCollection();
- List<String> retainedGroupIds =
deleteCandidateGroupIds(groupErrMap, groupList, collection);
- if (retainedGroupIds.isEmpty()) {
- return CompletableFuture.completedFuture(collection);
- }
+
CompletableFuture<DeleteGroupsResponseData.DeletableGroupResultCollection>
future = deleteShareGroups(topicPartition, groupList)
+ .thenCompose(groupErrMap -> {
+ DeleteGroupsResponseData.DeletableGroupResultCollection
collection = new DeleteGroupsResponseData.DeletableGroupResultCollection();
Review Comment:
`collection` would be better named according to what it's used for. How
about `deletableGroupResults`?
##########
share-coordinator/src/test/java/org/apache/kafka/coordinator/share/ShareCoordinatorShardTest.java:
##########
@@ -979,9 +979,7 @@ public void testDeleteStateSuccess() {
.setPartitions(List.of(new
DeleteShareGroupStateRequestData.PartitionData()
.setPartition(PARTITION)))));
- CoordinatorResult<DeleteShareGroupStateResponseData,
CoordinatorRecord> result = shard.deleteState(request);
-
- // apply a record in to verify delete
+ // Apply a record in to verify delete.
Review Comment:
nit: Grammar. "Apply a record to verify delete." would be better.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java:
##########
@@ -1067,25 +1067,27 @@ public
CompletableFuture<DeleteGroupsResponseData.DeletableGroupResultCollection
});
groupsByTopicPartition.forEach((topicPartition, groupList) -> {
-
CompletableFuture<DeleteGroupsResponseData.DeletableGroupResultCollection>
future = deleteShareGroups(topicPartition, groupList).thenCompose(groupErrMap
-> {
- DeleteGroupsResponseData.DeletableGroupResultCollection
collection = new DeleteGroupsResponseData.DeletableGroupResultCollection();
- List<String> retainedGroupIds =
deleteCandidateGroupIds(groupErrMap, groupList, collection);
- if (retainedGroupIds.isEmpty()) {
- return CompletableFuture.completedFuture(collection);
- }
+
CompletableFuture<DeleteGroupsResponseData.DeletableGroupResultCollection>
future = deleteShareGroups(topicPartition, groupList)
+ .thenCompose(groupErrMap -> {
+ DeleteGroupsResponseData.DeletableGroupResultCollection
collection = new DeleteGroupsResponseData.DeletableGroupResultCollection();
+ List<String> retainedGroupIds =
deleteCandidateGroupIds(groupErrMap, groupList, collection);
Review Comment:
`deleteCandidateGroupIds` is very obscure. It's removing groups whose
deletion failed and adding responses for them.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorShard.java:
##########
@@ -579,15 +579,16 @@ public
CoordinatorResult<DeleteGroupsResponseData.DeletableGroupResultCollection
* <p></p>
Review Comment:
nit: This pair of tags is just odd. I suggest just `<p>` or remove them both.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
