satishd commented on code in PR #16078:
URL: https://github.com/apache/kafka/pull/16078#discussion_r1630642185
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -1166,7 +1166,9 @@ class DynamicRemoteLogConfig(server: KafkaBroker) extends
BrokerReconfigurable w
override def validateReconfiguration(newConfig: KafkaConfig): Unit = {
newConfig.values.forEach { (k, v) =>
if (reconfigurableConfigs.contains(k)) {
- if
(k.equals(RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP))
{
+ if
(k.equals(RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP)
||
Review Comment:
I see that it is the existing logic for the existing config key
`RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP`for
the respective `Long` values.
Although all the valid keys are already checked by the earlier line `if
(reconfigurableConfigs.contains(k))`, they are getting repeated here to check
and cast the value as Long. Can we avoid double checking by removing the
earlier check `reconfigurableConfigs.contains(k)`?
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -1179,14 +1181,31 @@ class DynamicRemoteLogConfig(server: KafkaBroker)
extends BrokerReconfigurable w
}
override def reconfigure(oldConfig: KafkaConfig, newConfig: KafkaConfig):
Unit = {
- val oldValue =
oldConfig.getLong(RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP)
- val newValue =
newConfig.getLong(RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP)
- if (oldValue != newValue) {
- val remoteLogManager = server.remoteLogManagerOpt
- if (remoteLogManager.nonEmpty) {
+ def oldLongValue(k: String): Long = oldConfig.getLong(k)
+ def newLongValue(k: String): Long = newConfig.getLong(k)
+
+ def isChangedLongValue(k : String): Boolean = oldLongValue(k) !=
newLongValue(k)
+
+ val remoteLogManager = server.remoteLogManagerOpt
+ if (remoteLogManager.nonEmpty) {
+ if
(isChangedLongValue(RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP))
{
+ val oldValue =
oldLongValue(RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP)
+ val newValue =
newLongValue(RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP)
remoteLogManager.get.resizeCacheSize(newValue)
info(s"Dynamic remote log manager config:
${RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP}
updated, " +
s"old value: $oldValue, new value: $newValue")
+ } else if
(isChangedLongValue(RemoteLogManagerConfig.REMOTE_LOG_MANAGER_COPY_MAX_BYTES_PER_SECOND_PROP))
{
Review Comment:
This method should take care of updating any of the respective configs. That
means, it should individually check for each config by replacing `else if` with
`if`.
It is also good to add a test that updates multiple configs and check all of
them are getting updated.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
