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

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

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


##########
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());

Review Comment:
   nit: Noting that this first clause is explicitly rejecting the possibility of an idempotent replay of the same record a second time.  This makes sense, as it is unexpected if it should happen. 
    It might be good to call this out explicitly in a comment.



##########
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:
   +1



##########
metadata/src/test/java/org/apache/kafka/controller/MockAclControlManager.java:
##########
@@ -33,7 +33,7 @@
 public class MockAclControlManager extends AclControlManager {
     public MockAclControlManager(LogContext logContext,
                                  Optional<ClusterMetadataAuthorizer> authorizer) {
-        super(new SnapshotRegistry(logContext), authorizer);
+        super(new LogContext(), new SnapshotRegistry(logContext), authorizer);

Review Comment:
   Seems like it should be `s/new LogContext()/logContext/`?



##########
metadata/src/main/java/org/apache/kafka/controller/AclControlManager.java:
##########
@@ -64,12 +66,44 @@
  * completed, which is another reason the prepare / complete callbacks are needed.
  */
 public class AclControlManager {
+    static class Builder {
+        private LogContext logContext = null;
+        private SnapshotRegistry snapshotRegistry = null;
+        private Optional<ClusterMetadataAuthorizer> authorizer = Optional.empty();
+
+        Builder setLogContext(LogContext logContext) {
+            this.logContext = logContext;
+            return this;
+        }
+
+        Builder setSnapshotRegistry(SnapshotRegistry snapshotRegistry) {
+            this.snapshotRegistry = snapshotRegistry;
+            return this;
+        }
+
+        Builder setClusterMetadataAuthorizer(Optional<ClusterMetadataAuthorizer> authorizer) {
+            this.authorizer = authorizer;
+            return this;
+        }
+
+        AclControlManager build() {
+            if (logContext == null) logContext = new LogContext();

Review Comment:
   I agree this is a worthwhile question to consider.  I do note that the builders that already exist don't set anything special.  For example, from `ConfigurationControlManager.Builder.build()`:
   
   ```
               if (logContext == null) logContext = new LogContext();
   ```
   
   And all of these builder log contexts do get explicitly set in QuorumController (including the new ones via this PR), so the default will never be used.
   
   So my guess is the answer is no, it isn't worth doing.  Colin, WDYT?



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