[GitHub] [kafka] andymg3 commented on a diff in pull request #12145: KAFKA-13892: Dedupe RemoveAccessControlEntryRecord in deleteAcls response of AclControlManager

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

andymg3 commented on code in PR #12145:
URL: https://github.com/apache/kafka/pull/12145#discussion_r869680507


##########
metadata/src/main/java/org/apache/kafka/controller/AclControlManager.java:
##########
@@ -152,7 +154,11 @@ ControllerResult<List<AclDeleteResult>> deleteAcls(List<AclBindingFilter> filter
                 results.add(new AclDeleteResult(ApiError.fromThrowable(e).exception()));
             }
         }
-        return ControllerResult.atomicOf(records, results);
+        return ControllerResult.atomicOf(dedupeApiMessageAndVersion(records), results);
+    }
+
+    private List<ApiMessageAndVersion> dedupeApiMessageAndVersion(List<ApiMessageAndVersion> messages) {
+        return new HashSet<>(messages).stream().collect(Collectors.toList());

Review Comment:
   Good point. I think we could use a Set the whole way through except at the very end when we have to convert it to a List.
   
   Originally I was thinking about preserving the order of the messages as I did the de-dupe. I had the following at first:
   ```
   return new LinkedHashSet<>(messages).stream().collect(Collectors.toList());
   ```
   but then I realized the ordering shouldnt matter. So I think I can make the suggested change to just use a Set.



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