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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
