showuon commented on code in PR #16132: URL: https://github.com/apache/kafka/pull/16132#discussion_r1669549418
########## clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java: ########## @@ -93,6 +93,17 @@ public class TopicConfig { "deletes the old segments. Default value is -2, it represents `retention.bytes` value to be used. The effective value should always be " + "less than or equal to `retention.bytes` value."; + public static final String REMOTE_LOG_DISABLE_POLICY_RETAIN = "retain"; + public static final String REMOTE_LOG_DISABLE_POLICY_DELETE = "delete"; + + public static final String REMOTE_LOG_DISABLE_POLICY_CONFIG = "remote.log.disable.policy"; + + public static final String REMOTE_LOG_DISABLE_POLICY_DOC = String.format("Determines whether tiered data for a topic should be retained or " + + "deleted after tiered storage disablement on a topic. The two valid options are \"%s\" and \"%s\". If retain is " + + "selected then all data in remote will be kept post-disablement and will only be deleted when it breaches expiration " + + "thresholds. If delete is selected then the data will be made inaccessible immediately by advancing the log start offset and will be " + + "deleted asynchronously.", REMOTE_LOG_DISABLE_POLICY_RETAIN, REMOTE_LOG_DISABLE_POLICY_DELETE); Review Comment: nit: 1. "If _retain_ is selected" <-- the "retain" should be replaced with `REMOTE_LOG_DISABLE_POLICY_RETAIN` 2. "If _delete_ is selected" <-- the "delete" should be replaced with `REMOTE_LOG_DISABLE_POLICY_DELETE` ########## metadata/src/main/java/org/apache/kafka/metadata/KafkaConfigSchema.java: ########## @@ -166,9 +166,11 @@ public Map<String, ConfigEntry> resolveEffectiveTopicConfigs( ConfigDef configDef = configDefs.getOrDefault(ConfigResource.Type.TOPIC, EMPTY_CONFIG_DEF); HashMap<String, ConfigEntry> effectiveConfigs = new HashMap<>(); for (ConfigDef.ConfigKey configKey : configDef.configKeys().values()) { - ConfigEntry entry = resolveEffectiveTopicConfig(configKey, staticNodeConfig, - dynamicClusterConfigs, dynamicNodeConfigs, dynamicTopicConfigs); - effectiveConfigs.put(entry.name(), entry); + if (!configKey.internalConfig) { Review Comment: Could I know why we should do this change? For broken tests? I'm not 100% sure if this change changes some behavior or not. If so, I'd suggest we make the config as non-internal config. We plan to make it complete in v3.9.0 anyway. Thoughts? -- 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