You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/11/15 09:32:30 UTC

[GitHub] [kafka] zzccctv opened a new pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

zzccctv opened a new pull request #11496:
URL: https://github.com/apache/kafka/pull/11496


   kafka has duplicate configuration information log information printing during startup,repeated information printing will bring confusion to users.It is better to add log information before and after repeating the configuration information


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

[GitHub] [kafka] zzccctv edited a comment on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
zzccctv edited a comment on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-972630217


   @guozhangwang This suggestion looks very good, But there is a problem. The BrokerConfigHandler class also calls the updateDefaultConfig and updateBrrokerConfig functions respectively. If the validateonly parameter is added to updateCurrentConfig and false is set in updateDefaultConfig function. Although it solves the problem of repeatedly printing logs during startup, during dynamic configuration adjustment, It seems that this function will not work. Can we add a parameter to the processReconfiguration function to control the print log?I updated this PR,  if you agree with this idea, can you help me check it


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

[GitHub] [kafka] zzccctv edited a comment on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
zzccctv edited a comment on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-972630217


   @guozhangwang This suggestion looks very good, But there is a problem. The BrokerConfigHandler class also calls the updateDefaultConfig and updateBrrokerConfig functions respectively. It simply adds the validateonly parameter to updateCurrentConfig and sets false in the updateDefaultConfig function. Although it solves the problem of repeatedly printing logs during startup, during dynamic configuration adjustment, It seems that this function will not work. Can we add a parameter to the processReconfiguration function to control the print log?I updated this PR, if I agree with this idea, can you help me check it


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

[GitHub] [kafka] zzccctv commented on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
zzccctv commented on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-974112622


   @guozhangwang I updated it according to your suggestion, but I don't understand that it's necessary to print out all the configuration information again after the dynamic configuration adjustment is completed,Do you know the purpose of printing all configurations again?


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

[GitHub] [kafka] guozhangwang commented on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-971818060


   Ah thanks @zzccctv that helps.
   
   I'm now wondering if it is very useful to print the KafkaConfigs in `updateDefaultConfig` for the cluster-wide configs. What do you think about only printing once in `updateBrokerConfig` since that's the final configs to be used in this specific broker during initialization anyways.
   
   If you agree, we can augment the `updateCurrentConfig` function with the `validateOnly` flag, which would be passed in as false in `updateDefaultConfig` so we only print it once.


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

[GitHub] [kafka] zzccctv edited a comment on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
zzccctv edited a comment on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-974107345


   @guozhangwang I updated it according to your suggestion, but I don't understand that it's necessary to print out all the configuration information again after the dynamic configuration adjustment is completed


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

[GitHub] [kafka] zzccctv commented on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
zzccctv commented on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-971120974


   @guozhangwang During initialization, DynamicBrokerConfig will call updateDefaultConfig and updateBrrokerConfig, which call updateCurrentConfig respectively,In the updateCurrenceConfig function, because the validateonly value is false, the KafkaConfig in the processReconfiguration function prints duplicate log information


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

[GitHub] [kafka] zzccctv removed a comment on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
zzccctv removed a comment on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-974112622


   @guozhangwang I updated it according to your suggestion, but I don't understand that it's necessary to print out all the configuration information again after the dynamic configuration adjustment is completed,Do you know the purpose of printing all configurations again?


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

[GitHub] [kafka] zzccctv commented on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
zzccctv commented on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-974107345


   @guozhangwang I updated it according to your suggestion, but I don't understand that it's necessary to print out all the configuration information again after the dynamic configuration adjustment is completed,Do you know the purpose of printing all configurations again?


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

[GitHub] [kafka] zzccctv edited a comment on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
zzccctv edited a comment on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-972630217


   @guozhangwang This suggestion looks very good, But there is a problem. The BrokerConfigHandler class also calls the updateDefaultConfig and updateBrrokerConfig functions respectively. It simply adds the validateonly parameter to updateCurrentConfig and sets false in the updateDefaultConfig function. Although it solves the problem of repeatedly printing logs during startup, during dynamic configuration adjustment, It seems that this function will not work. Can we add a parameter to the processReconfiguration function to control the print log?I updated this PR,  if you agree with this idea, can you help me check it


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

[GitHub] [kafka] guozhangwang merged pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
guozhangwang merged pull request #11496:
URL: https://github.com/apache/kafka/pull/11496


   


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

[GitHub] [kafka] zzccctv commented on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
zzccctv commented on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-968806449


   @ijuma @guozhangwang Can you help me look at this problem?


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

[GitHub] [kafka] zzccctv commented on pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
zzccctv commented on pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#issuecomment-972630217


   @guozhangwang This suggestion looks very good, But there is a problem. The BrokerConfigHandler class also calls the updateDefaultConfig and updateBrrokerConfig functions respectively. It simply adds the validateonly parameter to updateCurrentConfig and sets false in the updateDefaultConfig function. Although it solves the problem of repeatedly printing logs during startup, during dynamic configuration adjustment, It seems that this function will not work. Can we add a parameter to the processReconfiguration function to control the print log?


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

[GitHub] [kafka] guozhangwang commented on a change in pull request #11496: KAFKA-13454: kafka has duplicate configuration information log information printin…

Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11496:
URL: https://github.com/apache/kafka/pull/11496#discussion_r752509125



##########
File path: core/src/main/scala/kafka/server/DynamicBrokerConfig.scala
##########
@@ -291,23 +291,23 @@ class DynamicBrokerConfig(private val kafkaConfig: KafkaConfig) extends Logging
     dynamicDefaultConfigs.clone()
   }
 
-  private[server] def updateBrokerConfig(brokerId: Int, persistentProps: Properties): Unit = CoreUtils.inWriteLock(lock) {
+  private[server] def updateBrokerConfig(brokerId: Int, persistentProps: Properties, doLog: Boolean = false): Unit = CoreUtils.inWriteLock(lock) {

Review comment:
       I think in order to not modify the behavior in `ConfigHandler`, we need to make the default `doLog` as true, and then explicitly set it in line 215 above as `false`.




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