[GitHub] [kafka] divijvaidya commented on a diff in pull request #13703: MINOR: Standardize controller log4j output for replaying records

["divijvaidya (via GitHub)" <gi...@apache.org>]

divijvaidya commented on code in PR #13703:
URL: https://github.com/apache/kafka/pull/13703#discussion_r1203778428


##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -473,8 +488,8 @@ public void replay(PartitionChangeRecord record) {
 
         if (record.removingReplicas() != null || record.addingReplicas() != null) {
             log.info("Replayed partition assignment change {} for topic {}", record, topicInfo.name);
-        } else if (log.isTraceEnabled()) {
-            log.trace("Replayed partition change {} for topic {}", record, topicInfo.name);
+        } else if (log.isDebugEnabled()) {

Review Comment:
   Do we really need this check? My understanding is that the parameterised messages (which we are using here) removes the requirement of this check. see: https://www.slf4j.org/faq.html#logging_performance 



##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -377,7 +377,19 @@ private ReplicationControlManager(
     }
 
     public void replay(TopicRecord record) {
-        topicsByName.put(record.name(), record.topicId());
+        Uuid existingUuid = topicsByName.put(record.name(), record.topicId());
+        if (existingUuid != null) {
+            // We don't currently support sending a second TopicRecord for the same topic name...
+            // unless, of course, there is a RemoveTopicRecord in between.
+            if (existingUuid.equals(record.topicId())) {
+                throw new RuntimeException("Found duplicate TopicRecord for " + record.name() +
+                        " with topic ID " + record.topicId());
+            } else {
+                throw new RuntimeException("Found duplicate TopicRecord for " + record.name() +
+                        " with a different ID than before. Previous ID was " + existingUuid +
+                        " and new ID is " + record.topicId());
+            }
+        }

Review Comment:
   1. This should perhaps go in a separate PR? It would help keeping commits as separate (thus helping in faster rollback if required)
   2. I am curious to understand where the RuntimeException gets logged (probably handled by event queue?)



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