kamalcph commented on code in PR #14161:
URL: https://github.com/apache/kafka/pull/14161#discussion_r1300091311
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java:
##########
@@ -500,22 +500,29 @@ public static void validateBrokerLogConfigValues(Map<?,
?> props,
* The default values should be extracted from the KafkaConfig.
* @param props The properties to be validated
*/
- private static void validateTopicLogConfigValues(Map<?, ?> props,
- boolean
isRemoteLogStorageSystemEnabled) {
+ public static void validateTopicLogConfigValues(Map<?, ?> props,
Review Comment:
If we are planning to validate only the case where tiered storage
functionality is enabled (or) disabled in the broker during restarts. Can we
use the `validateRemoteStorageOnlyIfSystemEnabled` instead of
`validateTopicLogConfigValues`?
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java:
##########
@@ -500,22 +500,29 @@ public static void validateBrokerLogConfigValues(Map<?,
?> props,
* The default values should be extracted from the KafkaConfig.
* @param props The properties to be validated
*/
- private static void validateTopicLogConfigValues(Map<?, ?> props,
- boolean
isRemoteLogStorageSystemEnabled) {
+ public static void validateTopicLogConfigValues(Map<?, ?> props,
+ boolean
isRemoteLogStorageSystemEnabled,
+ boolean
isReceivingConfigFromStore) {
validateValues(props);
boolean isRemoteLogStorageEnabled = (Boolean)
props.get(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG);
if (isRemoteLogStorageEnabled) {
-
validateRemoteStorageOnlyIfSystemEnabled(isRemoteLogStorageSystemEnabled);
- validateNoRemoteStorageForCompactedTopic(props);
- validateRemoteStorageRetentionSize(props);
- validateRemoteStorageRetentionTime(props);
+
validateRemoteStorageOnlyIfSystemEnabled(isRemoteLogStorageSystemEnabled,
isReceivingConfigFromStore);
+ if (!isReceivingConfigFromStore) {
+ validateNoRemoteStorageForCompactedTopic(props);
+ validateRemoteStorageRetentionSize(props);
+ validateRemoteStorageRetentionTime(props);
+ }
}
}
- private static void validateRemoteStorageOnlyIfSystemEnabled(boolean
isRemoteLogStorageSystemEnabled) {
+ private static void validateRemoteStorageOnlyIfSystemEnabled(boolean
isRemoteLogStorageSystemEnabled, boolean isReceivingConfigFromStore) {
if (!isRemoteLogStorageSystemEnabled) {
- throw new ConfigException("Tiered Storage functionality is
disabled in the broker. " +
- "Topic cannot be configured with remote log storage.");
+ if (isReceivingConfigFromStore) {
Review Comment:
Can we update the message such that it's same for both the cases?
If the user disables the tiered storage functionality in the broker and
restarts the node, and if any topic is left with remote storage enabled, then
the same error message can be reused.
--
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]
