divijvaidya commented on code in PR #14114:
URL: https://github.com/apache/kafka/pull/14114#discussion_r1281624661
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java:
##########
@@ -102,49 +102,14 @@ public String topicWarningMessage(String topicName) {
public static class RemoteLogConfig {
Review Comment:
> Do you have suggestion on how to add this validation?
Places such as `KafkaApis.scala`, `ControllerApis.scala` and
`ZkAdminManager` already have access to the `KafkaConfig` instance. When
`createTopic` or `incrementAlterConfig` API is called, they call
`LogConfig.validate()` today. Instead, they should do something like:
```
val defaultConfigs = kafkaConfig.extractLogConfigMap
LogConfig.validate(newConfigs, defaultConfigs) // new method
```
You will need to make the following changes:
1. Add the following line to `KafkaConfig.extractLogConfigMap()`
`logProps.put(RemoteLogManagerConfig.REMOTE_LOG_STORAGE_SYSTEM_ENABLE_PROP,
remoteLogManagerConfig.enableRemoteStorageSystem())`.
2. At places where we call `LogConfig.validate()` such as
`AdminZkClient.validateTopicConfig()`, use the new LogConfig.validate() method.
You can also pass KafkaConfig to AdminZkClient via ZkAdminManager ->
AdminZkClient.validateTopicConfig(). Similarly for KRaft.
3. Inside the new `LogConfig.validate()` method, add the check that we want
i.e. if system level remote storage is not enabled, then we cannot enable topic
level remote config.
--
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]
