[GitHub] [kafka] niket-goel commented on a change in pull request #11310: KAFKA-13279: Implement CreateTopicsPolicy for KRaft

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

niket-goel commented on a change in pull request #11310:
URL: https://github.com/apache/kafka/pull/11310#discussion_r710331074



##########
File path: metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java
##########
@@ -465,6 +485,13 @@ private ApiError createTopic(CreatableTopic topic,
                     "Unable to replicate the partition " + replicationFactor +
                         " time(s): " + e.getMessage());
             }
+            ApiError error = maybeCheckCreateTopicPolicy(() -> {
+                Map<String, String> configs = new HashMap<>();
+                topic.configs().forEach(config -> configs.put(config.name(), config.value()));
+                return new CreateTopicPolicy.RequestMetadata(
+                    topic.name(), numPartitions, replicationFactor, null, configs);
+            });
+            if (error.isFailure()) return error;
         }
         Uuid topicId = Uuid.randomUuid();

Review comment:
       nit: 
   Would it be more readable to combine the two calls to `maybeCheckCreateTopicPolicy` and place it after the if/else blocks?

##########
File path: clients/src/main/java/org/apache/kafka/server/policy/CreateTopicPolicy.java
##########
@@ -104,6 +105,24 @@ public Short replicationFactor() {
             return configs;
         }
 
+        @Override
+        public int hashCode() {
+            return Objects.hash(topic, numPartitions, replicationFactor,
+                replicasAssignments, configs);
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) return true;
+            if (o == null || getClass() != o.getClass()) return false;
+            RequestMetadata other = (RequestMetadata) o;
+            return topic.equals(other.topic) &&
+                Objects.equals(numPartitions, other.numPartitions) &&
+                Objects.equals(replicationFactor, other.replicationFactor) &&
+                Objects.equals(replicasAssignments, other.replicasAssignments) &&
+                configs.equals(other.configs);

Review comment:
       Questions for my own understanding:
   Is there a particular reason we are not using the Object.equals method to compare, which is also a map?




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