Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
satishd commented on PR #16353: URL: https://github.com/apache/kafka/pull/16353#issuecomment-2173081649 >This require a good amount of refactoring in RemoteLogManagerConfig class. Shall we continue with this patch to backport it to 3.8 branch? Or, do we have to refactor the code and land the changes only in trunk/3.9 branch? We can make the suggested [improvements](https://github.com/apache/kafka/pull/16353#pullrequestreview-2121953295) as a followup in trunk. Filed a followup https://issues.apache.org/jira/browse/KAFKA-16976. -- 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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
kamalcph commented on PR #16353: URL: https://github.com/apache/kafka/pull/16353#issuecomment-2173039397 > LGTM! Could you mention that this issue only happened in ZK mode in the PR description? Thanks. done. -- 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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
showuon commented on code in PR #16353: URL: https://github.com/apache/kafka/pull/16353#discussion_r1642569554 ## core/src/main/java/kafka/log/remote/RemoteLogManager.java: ## @@ -226,7 +227,8 @@ public RemoteLogManager(RemoteLogManagerConfig rlmConfig, rlmCopyQuotaManager = createRLMCopyQuotaManager(); rlmFetchQuotaManager = createRLMFetchQuotaManager(); -indexCache = new RemoteIndexCache(rlmConfig.remoteLogIndexFileCacheTotalSizeBytes(), remoteLogStorageManager, logDir); +RemoteLogManagerConfig rlmConfig = config.remoteLogManagerConfig(); +indexCache = new RemoteIndexCache(config.remoteLogIndexFileCacheTotalSizeBytes(), remoteLogStorageManager, logDir); Review Comment: Yes, I confirmed in KRaft, it won't have this issue. -- 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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
kamalcph commented on PR #16353: URL: https://github.com/apache/kafka/pull/16353#issuecomment-2172425056 > We should avoid passing KafkaConfig to RemoteLogManagerConfig and we need to make sure whenever KafkaConfig gets updated RemoteLogManagerConfig gets updated. With the current changes, it is happening in two different ways that we should avoid. This require a good amount of refactoring in RemoteLogManagerConfig class. Shall we continue with this patch to backport it to 3.8 branch? Or, do we need to refactor the code and land the changes only in trunk/3.9 branch? -- 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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
kamalcph commented on PR #16353: URL: https://github.com/apache/kafka/pull/16353#issuecomment-2172419234 yes, kraft config updates looks good to me. The DynamicConfigPublisher will eventually call the BrokerReconfigurable#reconfigure, then the configs gets updated on those respective components. -- 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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
chia7712 commented on PR #16353: URL: https://github.com/apache/kafka/pull/16353#issuecomment-2172396287 > Update RemoteLogManagerConfig with the respective dynamic configs when a broker is initialized. In this case, the respective configs in RemoteLogManagerConfig should get updated and there won't be any dangling old config references. there was a discussion about "can we see the (latest) dynamic configs when a broker is initialized" (https://github.com/apache/kafka/pull/16353#discussion_r1641654171). In kraft, we create `remoteLogManager`[0] before updating dynamic configs [1]. Hence, it seems it is possible that `remoteLogManager` still see the static configs. This behavior is different to zk mode. Those remoteLogManager configs [3] can be updated later when publishing metadata (latest configs), so that is possibly be fine (?). Fixing the startup order seems be a big issue, and maybe it is hard to guarantee "all" components can see latest configs in kraft. [0] https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/BrokerServer.scala#L208 [1] https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/BrokerServer.scala#L499 [2] https://github.com/apache/kafka/blob/4a37c2e18f4658123557fc48957a310d53d31bac/core/src/main/scala/kafka/server/DynamicBrokerConfig.scala#L1178 -- 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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
chia7712 commented on code in PR #16353: URL: https://github.com/apache/kafka/pull/16353#discussion_r1641770712 ## core/src/main/java/kafka/log/remote/RemoteLogManager.java: ## @@ -226,7 +227,8 @@ public RemoteLogManager(RemoteLogManagerConfig rlmConfig, rlmCopyQuotaManager = createRLMCopyQuotaManager(); rlmFetchQuotaManager = createRLMFetchQuotaManager(); -indexCache = new RemoteIndexCache(rlmConfig.remoteLogIndexFileCacheTotalSizeBytes(), remoteLogStorageManager, logDir); +RemoteLogManagerConfig rlmConfig = config.remoteLogManagerConfig(); +indexCache = new RemoteIndexCache(config.remoteLogIndexFileCacheTotalSizeBytes(), remoteLogStorageManager, logDir); Review Comment: > I have tested the patch only with ZooKeeper. I think the behavior should be similar for KRaftMetadataCache/ConfigRepository. That is a good point, and maybe they do have something difference. https://github.com/apache/kafka/blob/bcf781230e750fd5efbf276e984f2875bf9fa683/core/src/main/scala/kafka/server/DynamicBrokerConfig.scala#L228 in kraft, `zkClientOpt` is none so it does not update it with dynamical parts. -- 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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
kamalcph commented on code in PR #16353:
URL: https://github.com/apache/kafka/pull/16353#discussion_r1641757420
##
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##
@@ -412,7 +418,7 @@ public void onLeadershipChange(Set
partitionsBecomeLeader,
Map topicIds) {
LOGGER.debug("Received leadership changes for leaders: {} and
followers: {}", partitionsBecomeLeader, partitionsBecomeFollower);
-if (this.rlmConfig.isRemoteStorageSystemEnabled() &&
!isRemoteLogManagerConfigured()) {
+if (config.remoteLogManagerConfig().isRemoteStorageSystemEnabled() &&
!isRemoteLogManagerConfigured()) {
Review Comment:
yes, this is the main issue. We access the remote configurations using
`KafkaConfig.remoteLogManagerConfig.xyz()` but it reflects to the value in the
static server.properties file not the dynamically updated one.
So, changed the usages to `KafkaConfig.xyz()` to get the dynamically updated
value.
--
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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
kamalcph commented on code in PR #16353: URL: https://github.com/apache/kafka/pull/16353#discussion_r1641760409 ## core/src/main/java/kafka/log/remote/RemoteLogManager.java: ## @@ -226,7 +227,8 @@ public RemoteLogManager(RemoteLogManagerConfig rlmConfig, rlmCopyQuotaManager = createRLMCopyQuotaManager(); rlmFetchQuotaManager = createRLMFetchQuotaManager(); -indexCache = new RemoteIndexCache(rlmConfig.remoteLogIndexFileCacheTotalSizeBytes(), remoteLogStorageManager, logDir); +RemoteLogManagerConfig rlmConfig = config.remoteLogManagerConfig(); +indexCache = new RemoteIndexCache(config.remoteLogIndexFileCacheTotalSizeBytes(), remoteLogStorageManager, logDir); Review Comment: > 1. IIRC, the dynamical configs are loaded by another thread, and hence we may NOT see the latest configs, which were updated dynamically, in creating RemoteLogManager, right? No, KafkaServer/BrokerServer does `config.dynamicConfig.initialize` before creating the RemoteLogManager instance so the dynamic configs gets updated in the `KafkaConfig` object but not in the `KafkaConfig.remoteLogManagerConfig()`. I have tested the patch only with ZooKeeper. I think the behavior should be similar for KRaftMetadataCache/ConfigRepository. > 2. those configs (remote.log.manager.copy.max.bytes.per.second, remote.log.manager.fetch.max.bytes.per.second) can be updated by reconfigure process, so it should be fine to initialize them with "stale" (static) configs after broker restart, right? This is correct, the `reconfigure` updates the dynamic value but we are referring to the static value as explained above. -- 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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
kamalcph commented on code in PR #16353: URL: https://github.com/apache/kafka/pull/16353#discussion_r1641760409 ## core/src/main/java/kafka/log/remote/RemoteLogManager.java: ## @@ -226,7 +227,8 @@ public RemoteLogManager(RemoteLogManagerConfig rlmConfig, rlmCopyQuotaManager = createRLMCopyQuotaManager(); rlmFetchQuotaManager = createRLMFetchQuotaManager(); -indexCache = new RemoteIndexCache(rlmConfig.remoteLogIndexFileCacheTotalSizeBytes(), remoteLogStorageManager, logDir); +RemoteLogManagerConfig rlmConfig = config.remoteLogManagerConfig(); +indexCache = new RemoteIndexCache(config.remoteLogIndexFileCacheTotalSizeBytes(), remoteLogStorageManager, logDir); Review Comment: > 1. IIRC, the dynamical configs are loaded by another thread, and hence we may NOT see the latest configs, which were updated dynamically, in creating RemoteLogManager, right? No, KafkaServer/BrokerServer does `config.dynamicConfig.initialize` before creating the RemoteLogManager instance so the dynamic configs gets updated in the `KafkaConfig` object but not in the `KafkaConfig.remoteLogManagerConfig()`. I have tested the patch only with ZooKeeper. I think the behavior should be similar of KRaftMetadataCache/ConfigRepository. -- 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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
kamalcph commented on code in PR #16353:
URL: https://github.com/apache/kafka/pull/16353#discussion_r1641757420
##
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##
@@ -412,7 +418,7 @@ public void onLeadershipChange(Set
partitionsBecomeLeader,
Map topicIds) {
LOGGER.debug("Received leadership changes for leaders: {} and
followers: {}", partitionsBecomeLeader, partitionsBecomeFollower);
-if (this.rlmConfig.isRemoteStorageSystemEnabled() &&
!isRemoteLogManagerConfigured()) {
+if (config.remoteLogManagerConfig().isRemoteStorageSystemEnabled() &&
!isRemoteLogManagerConfigured()) {
Review Comment:
yes, this is the main issue. We access the remote configurations using
`KafkaConfig.remoteLogManagerConfig.xyz()` but it reflects to the config in the
static file not the dynamically updated one.
So, changed the usages to `KafkaConfig.xyz()` to use the dynamically updated
value.
--
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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
kamalcph commented on code in PR #16353:
URL: https://github.com/apache/kafka/pull/16353#discussion_r1641757420
##
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##
@@ -412,7 +418,7 @@ public void onLeadershipChange(Set
partitionsBecomeLeader,
Map topicIds) {
LOGGER.debug("Received leadership changes for leaders: {} and
followers: {}", partitionsBecomeLeader, partitionsBecomeFollower);
-if (this.rlmConfig.isRemoteStorageSystemEnabled() &&
!isRemoteLogManagerConfigured()) {
+if (config.remoteLogManagerConfig().isRemoteStorageSystemEnabled() &&
!isRemoteLogManagerConfigured()) {
Review Comment:
yes, this is the main issue. We access the remote configurations using
`KafkaConfig.remoteLogManagerConfig.xyz()` but it reflects to the config in the
static file not the dynamically updated one.
--
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
Re: [PR] KAFKA-15265: Reapply dynamic remote configs after broker restart [kafka]
chia7712 commented on code in PR #16353:
URL: https://github.com/apache/kafka/pull/16353#discussion_r1641654171
##
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##
@@ -226,7 +227,8 @@ public RemoteLogManager(RemoteLogManagerConfig rlmConfig,
rlmCopyQuotaManager = createRLMCopyQuotaManager();
rlmFetchQuotaManager = createRLMFetchQuotaManager();
-indexCache = new
RemoteIndexCache(rlmConfig.remoteLogIndexFileCacheTotalSizeBytes(),
remoteLogStorageManager, logDir);
+RemoteLogManagerConfig rlmConfig = config.remoteLogManagerConfig();
+indexCache = new
RemoteIndexCache(config.remoteLogIndexFileCacheTotalSizeBytes(),
remoteLogStorageManager, logDir);
Review Comment:
not sure whether I have caught the context, so please feel free to correct
me.
1. IIRC, the dynamical configs are loaded by another thread, and hence we
may NOT see the latest configs, which were updated dynamically, in creating
`RemoteLogManager`, right?
2. those configs (`remote.log.manager.copy.max.bytes.per.second`,
`remote.log.manager.fetch.max.bytes.per.second`) can be updated by
`reconfigure` process, so it should be fine to initialize them with "stale"
(static) configs after broker restart, right?
##
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##
@@ -412,7 +418,7 @@ public void onLeadershipChange(Set
partitionsBecomeLeader,
Map topicIds) {
LOGGER.debug("Received leadership changes for leaders: {} and
followers: {}", partitionsBecomeLeader, partitionsBecomeFollower);
-if (this.rlmConfig.isRemoteStorageSystemEnabled() &&
!isRemoteLogManagerConfigured()) {
+if (config.remoteLogManagerConfig().isRemoteStorageSystemEnabled() &&
!isRemoteLogManagerConfigured()) {
Review Comment:
this is unrelated to this PR, but `config.remoteLogManagerConfig` always
return the same config even though the `config` is updated dynamically. That
seems to be error-prone, since it means the "updated" config can return
"remoteLogManagerConfig" with "previous" configs.
--
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
