kamalcph commented on code in PR #17859:
URL: https://github.com/apache/kafka/pull/17859#discussion_r1849800474


##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -1167,6 +1167,22 @@ class DynamicRemoteLogConfig(server: KafkaBroker) 
extends BrokerReconfigurable w
           throw new ConfigException(s"$errorMsg, value should be at least 1")
         }
       }
+
+      if 
(RemoteLogManagerConfig.REMOTE_LOG_MANAGER_COPIER_THREAD_POOL_SIZE_PROP.equals(k)
 ||
+        
RemoteLogManagerConfig.REMOTE_LOG_MANAGER_EXPIRATION_THREAD_POOL_SIZE_PROP.equals(k)
 ||
+        RemoteLogManagerConfig.REMOTE_LOG_READER_THREADS_PROP.equals(k)) {
+        val newValue = v.asInstanceOf[Int]
+        val oldValue = server.config.getInt(k)
+        if (newValue != oldValue) {
+          val errorMsg = s"Dynamic thread count update validation failed for 
$k=$v"
+          if (newValue <= 0)

Review Comment:
   > The configuration can be updated to its default value of -1 through a 
static configuration update.
   
   This is allowed. Only the newValue which is being dynamically updated to -1 
is not allowed. 
   
   > Is this an intended change that does not allow updating it to the default 
value (-1) through dynamic configs?
   
   Yes, we made those thread-pools default to -1 to derive the value from 
`REMOTE_LOG_MANAGER_THREAD_POOL_SIZE_PROP`. Once those properties are 
configured to be a non-negative number, then we expect the user to provide 
subsequent valid value when updating it dynamically.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to