[GitHub] [kafka] showuon commented on a change in pull request #10843: MINOR: Log formatting for exceptions during configuration related operations

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

showuon commented on a change in pull request #10843:
URL: https://github.com/apache/kafka/pull/10843#discussion_r650888452



##########
File path: core/src/main/scala/kafka/server/DynamicBrokerConfig.scala
##########
@@ -469,7 +469,7 @@ class DynamicBrokerConfig(private val kafkaConfig: KafkaConfig) extends Logging
         }
         invalidProps.keys.foreach(props.remove)
         val configSource = if (perBrokerConfig) "broker" else "default cluster"
-        error(s"Dynamic $configSource config contains invalid values: $invalidProps, these configs will be ignored", e)
+        error(s"Dynamic $configSource config contains invalid values in: ${invalidProps.keys}, these configs will be ignored", e)

Review comment:
       Why should we log only `invalidProps.keys` here? I think we should log `key:value` pair to users to know what invalid key and value they provided. What do you think?
   
   Also same questions to above 2 changes. Thanks.

##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##########
@@ -174,11 +174,15 @@ public int hashCode() {
         return result;
     }
 
+    /**
+     * Override toString to redact sensitive value.
+     * WARNING, user should be responsible to set correct "senstive" field for each config entry.

Review comment:
       **sensitive** is typo. Also, maybe we can change this sentense: 
   
   user should be responsible to set correct "senstive" field for each config entry.
   -> user should be responsible to set **the** correct **"isSensitive"** field for each config entry.

##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##########
@@ -174,11 +174,15 @@ public int hashCode() {
         return result;
     }
 
+    /**
+     * Override toString to redact sensitive value.
+     * WARNING, user should be responsible to set correct "senstive" field for each config entry.
+     */
     @Override
     public String toString() {
         return "ConfigEntry(" +
                 "name=" + name +
-                ", value=" + value +
+                ", value=" + (isSensitive ? "Redacted" : value) +

Review comment:
       Nice fix!




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org