You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "azhar2407 (via GitHub)" <gi...@apache.org> on 2025/04/01 18:34:42 UTC
[PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
azhar2407 opened a new pull request, #19345:
URL: https://github.com/apache/kafka/pull/19345
As of 3.9, Kafka allows disabling remote storage on a topic after it was enabled. It allows subsequent enabling and disabling too.
However the documentation says otherwise and needs to be corrected.
Doc: https://kafka.apache.org/39/documentation/#topicconfigs_remote.storage.enable
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "azhar2407 (via GitHub)" <gi...@apache.org>.
azhar2407 commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2030339041
##########
clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java:
##########
@@ -80,8 +80,9 @@ public class TopicConfig {
"Moreover, it triggers the rolling of new segment if the retention.ms condition is satisfied.";
public static final String REMOTE_LOG_STORAGE_ENABLE_CONFIG = "remote.storage.enable";
- public static final String REMOTE_LOG_STORAGE_ENABLE_DOC = "To enable tiered storage for a topic, set this configuration as true. " +
- "You can not disable this config once it is enabled. It will be provided in future versions.";
+ public static final String REMOTE_LOG_STORAGE_ENABLE_DOC = "To enable tiered storage for a topic, set this configuration to true. " +
+ "To disable tiered storage for a topic that has it enabled, set this configuration to false. " +
+ "When disabling, you must also set \"remote.log.delete.on.disable\" to true.";
Review Comment:
Corrected it. Thanks for pointing out
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #19345:
URL: https://github.com/apache/kafka/pull/19345#issuecomment-2800330421
@m1a2st , do you have any other comments on this PR?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2024038450
##########
clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java:
##########
@@ -81,7 +81,8 @@ public class TopicConfig {
public static final String REMOTE_LOG_STORAGE_ENABLE_CONFIG = "remote.storage.enable";
public static final String REMOTE_LOG_STORAGE_ENABLE_DOC = "To enable tiered storage for a topic, set this configuration as true. " +
- "You can not disable this config once it is enabled. It will be provided in future versions.";
+ "Disable tiered storage by setting this configuration to false for a tiered storage enabled topic. Setting this configuration to false " +
+ "must be accompanied by setting `remote.log.delete.on.disable` to true";
Review Comment:
Maybe we can make it clearer like this:
To enable tiered storage for a topic, set this configuration to true. To disable tiered storage for a topic that has it enabled, set this configuration to false. When disabling, you must also set <code>remote.log.delete.on.disable</code> to true.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2030345697
##########
core/src/test/scala/integration/kafka/admin/RemoteTopicCrudTest.scala:
##########
@@ -441,7 +441,29 @@ class RemoteTopicCrudTest extends IntegrationTestHarness {
AlterConfigOp.OpType.SET),
))
assertThrowsException(classOf[InvalidConfigurationException],
- () => admin.incrementalAlterConfigs(configs).all().get(), "Disabling remote storage feature on the topic level is not supported.")
+ () => admin.incrementalAlterConfigs(configs).all().get(), "It is invalid to disable remote storage without deleting remote data. " +
+ "If you want to keep the remote data and turn to read only, please set `remote.storage.enable=true,remote.log.copy.disable=true`. " +
+ "If you want to disable remote storage and delete all remote data, please set `remote.storage.enable=false,remote.log.delete.on.disable=true`.")
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = Array("kraft"))
+ def testUpdateTopicConfigWithDisablingRemoteStorageWithDeleteOnDisable(quorum: String): Unit = {
+ val admin = createAdminClient()
+ val topicConfig = new Properties
+ topicConfig.setProperty(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "true")
+ TestUtils.createTopicWithAdmin(admin, testTopicName, brokers, controllerServers, numPartitions, numReplicationFactor,
+ topicConfig = topicConfig)
+
+ val configs = new util.HashMap[ConfigResource, util.Collection[AlterConfigOp]]()
+ configs.put(new ConfigResource(ConfigResource.Type.TOPIC, testTopicName),
+ util.Arrays.asList(
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "false"),
+ AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_DELETE_ON_DISABLE_CONFIG, "true"),
+ AlterConfigOp.OpType.SET)
+ ))
+ verifyRemoteLogTopicConfigs(topicConfig)
Review Comment:
It's better to add `cleanTest` like `./gradlew cleanTest core:test --tests RemoteTopicCrudTest`.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2041319621
##########
core/src/test/scala/integration/kafka/admin/RemoteTopicCrudTest.scala:
##########
@@ -441,7 +441,36 @@ class RemoteTopicCrudTest extends IntegrationTestHarness {
AlterConfigOp.OpType.SET),
))
assertThrowsException(classOf[InvalidConfigurationException],
- () => admin.incrementalAlterConfigs(configs).all().get(), "Disabling remote storage feature on the topic level is not supported.")
+ () => admin.incrementalAlterConfigs(configs).all().get(), "It is invalid to disable remote storage without deleting remote data. " +
+ "If you want to keep the remote data and turn to read only, please set `remote.storage.enable=true,remote.log.copy.disable=true`. " +
+ "If you want to disable remote storage and delete all remote data, please set `remote.storage.enable=false,remote.log.delete.on.disable=true`.")
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = Array("kraft"))
+ def testUpdateTopicConfigWithDisablingRemoteStorageWithDeleteOnDisable(quorum: String): Unit = {
Review Comment:
Ah, you're right. Thanks for the explanation.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2040584325
##########
core/src/test/scala/integration/kafka/admin/RemoteTopicCrudTest.scala:
##########
@@ -441,7 +441,36 @@ class RemoteTopicCrudTest extends IntegrationTestHarness {
AlterConfigOp.OpType.SET),
))
assertThrowsException(classOf[InvalidConfigurationException],
- () => admin.incrementalAlterConfigs(configs).all().get(), "Disabling remote storage feature on the topic level is not supported.")
+ () => admin.incrementalAlterConfigs(configs).all().get(), "It is invalid to disable remote storage without deleting remote data. " +
+ "If you want to keep the remote data and turn to read only, please set `remote.storage.enable=true,remote.log.copy.disable=true`. " +
+ "If you want to disable remote storage and delete all remote data, please set `remote.storage.enable=false,remote.log.delete.on.disable=true`.")
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = Array("kraft"))
+ def testUpdateTopicConfigWithDisablingRemoteStorageWithDeleteOnDisable(quorum: String): Unit = {
Review Comment:
We already have `testCreateRemoteTopicWithCopyDisabledAndDeleteOnDisable` to test the same thing. I think this is not needed.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "azhar2407 (via GitHub)" <gi...@apache.org>.
azhar2407 commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2025309940
##########
clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java:
##########
@@ -81,7 +81,8 @@ public class TopicConfig {
public static final String REMOTE_LOG_STORAGE_ENABLE_CONFIG = "remote.storage.enable";
public static final String REMOTE_LOG_STORAGE_ENABLE_DOC = "To enable tiered storage for a topic, set this configuration as true. " +
- "You can not disable this config once it is enabled. It will be provided in future versions.";
+ "Disable tiered storage by setting this configuration to false for a tiered storage enabled topic. Setting this configuration to false " +
+ "must be accompanied by setting `remote.log.delete.on.disable` to true";
Review Comment:
Corrected it
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2028124270
##########
clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java:
##########
@@ -80,8 +80,9 @@ public class TopicConfig {
"Moreover, it triggers the rolling of new segment if the retention.ms condition is satisfied.";
public static final String REMOTE_LOG_STORAGE_ENABLE_CONFIG = "remote.storage.enable";
- public static final String REMOTE_LOG_STORAGE_ENABLE_DOC = "To enable tiered storage for a topic, set this configuration as true. " +
- "You can not disable this config once it is enabled. It will be provided in future versions.";
+ public static final String REMOTE_LOG_STORAGE_ENABLE_DOC = "To enable tiered storage for a topic, set this configuration to true. " +
+ "To disable tiered storage for a topic that has it enabled, set this configuration to false. " +
+ "When disabling, you must also set \"remote.log.delete.on.disable\" to true.";
Review Comment:
Please change to use code block `<code>remote.log.delete.on.disable</code>`, most configs use code block in Kafka document
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #19345:
URL: https://github.com/apache/kafka/pull/19345#issuecomment-2800366299
Thanks for the fix @azhar2407 !
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "azhar2407 (via GitHub)" <gi...@apache.org>.
azhar2407 commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2027531080
##########
core/src/test/scala/integration/kafka/admin/RemoteTopicCrudTest.scala:
##########
@@ -441,7 +441,29 @@ class RemoteTopicCrudTest extends IntegrationTestHarness {
AlterConfigOp.OpType.SET),
))
assertThrowsException(classOf[InvalidConfigurationException],
- () => admin.incrementalAlterConfigs(configs).all().get(), "Disabling remote storage feature on the topic level is not supported.")
+ () => admin.incrementalAlterConfigs(configs).all().get(), "It is invalid to disable remote storage without deleting remote data. " +
+ "If you want to keep the remote data and turn to read only, please set `remote.storage.enable=true,remote.log.copy.disable=true`. " +
+ "If you want to disable remote storage and delete all remote data, please set `remote.storage.enable=false,remote.log.delete.on.disable=true`.")
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = Array("kraft"))
+ def testUpdateTopicConfigWithDisablingRemoteStorageWithDeleteOnDisable(quorum: String): Unit = {
+ val admin = createAdminClient()
+ val topicConfig = new Properties
+ topicConfig.setProperty(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "true")
+ TestUtils.createTopicWithAdmin(admin, testTopicName, brokers, controllerServers, numPartitions, numReplicationFactor,
+ topicConfig = topicConfig)
+
+ val configs = new util.HashMap[ConfigResource, util.Collection[AlterConfigOp]]()
+ configs.put(new ConfigResource(ConfigResource.Type.TOPIC, testTopicName),
+ util.Arrays.asList(
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "false"),
+ AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_DELETE_ON_DISABLE_CONFIG, "true"),
+ AlterConfigOp.OpType.SET)
+ ))
+ verifyRemoteLogTopicConfigs(topicConfig)
Review Comment:
My bad. Added it now
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "azhar2407 (via GitHub)" <gi...@apache.org>.
azhar2407 commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2030339979
##########
core/src/test/scala/integration/kafka/admin/RemoteTopicCrudTest.scala:
##########
@@ -441,7 +441,29 @@ class RemoteTopicCrudTest extends IntegrationTestHarness {
AlterConfigOp.OpType.SET),
))
assertThrowsException(classOf[InvalidConfigurationException],
- () => admin.incrementalAlterConfigs(configs).all().get(), "Disabling remote storage feature on the topic level is not supported.")
+ () => admin.incrementalAlterConfigs(configs).all().get(), "It is invalid to disable remote storage without deleting remote data. " +
+ "If you want to keep the remote data and turn to read only, please set `remote.storage.enable=true,remote.log.copy.disable=true`. " +
+ "If you want to disable remote storage and delete all remote data, please set `remote.storage.enable=false,remote.log.delete.on.disable=true`.")
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = Array("kraft"))
+ def testUpdateTopicConfigWithDisablingRemoteStorageWithDeleteOnDisable(quorum: String): Unit = {
+ val admin = createAdminClient()
+ val topicConfig = new Properties
+ topicConfig.setProperty(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "true")
+ TestUtils.createTopicWithAdmin(admin, testTopicName, brokers, controllerServers, numPartitions, numReplicationFactor,
+ topicConfig = topicConfig)
+
+ val configs = new util.HashMap[ConfigResource, util.Collection[AlterConfigOp]]()
+ configs.put(new ConfigResource(ConfigResource.Type.TOPIC, testTopicName),
+ util.Arrays.asList(
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "false"),
+ AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_DELETE_ON_DISABLE_CONFIG, "true"),
+ AlterConfigOp.OpType.SET)
+ ))
+ verifyRemoteLogTopicConfigs(topicConfig)
Review Comment:
Excuse my inexperience but somehow there are no failures when I run these tests locally using command `./gradlew core:test --tests RemoteTopicCrudTest `. Any way, I have updated the test to pass the changed configs for validation. Ran the test again and everything passes. Hopeful build/tests will succeed now
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2024038450
##########
clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java:
##########
@@ -81,7 +81,8 @@ public class TopicConfig {
public static final String REMOTE_LOG_STORAGE_ENABLE_CONFIG = "remote.storage.enable";
public static final String REMOTE_LOG_STORAGE_ENABLE_DOC = "To enable tiered storage for a topic, set this configuration as true. " +
- "You can not disable this config once it is enabled. It will be provided in future versions.";
+ "Disable tiered storage by setting this configuration to false for a tiered storage enabled topic. Setting this configuration to false " +
+ "must be accompanied by setting `remote.log.delete.on.disable` to true";
Review Comment:
Maybe we can make it clearer like this:
To enable tiered storage for a topic, set this configuration to true. To disable tiered storage for a topic that has it enabled, set this configuration to false. When disabling, you must also set <code>remote.log.delete.on.disable</code> to true."
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon merged PR #19345:
URL: https://github.com/apache/kafka/pull/19345
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2024038450
##########
clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java:
##########
@@ -81,7 +81,8 @@ public class TopicConfig {
public static final String REMOTE_LOG_STORAGE_ENABLE_CONFIG = "remote.storage.enable";
public static final String REMOTE_LOG_STORAGE_ENABLE_DOC = "To enable tiered storage for a topic, set this configuration as true. " +
- "You can not disable this config once it is enabled. It will be provided in future versions.";
+ "Disable tiered storage by setting this configuration to false for a tiered storage enabled topic. Setting this configuration to false " +
+ "must be accompanied by setting `remote.log.delete.on.disable` to true";
Review Comment:
Maybe we can make it clearer like this:
To enable tiered storage for a topic, set this configuration to true.
To disable tiered storage for a topic that has it enabled, set this configuration to false.
When disabling, you must also set <code>remote.log.delete.on.disable</code> to true."
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "azhar2407 (via GitHub)" <gi...@apache.org>.
azhar2407 commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2030339979
##########
core/src/test/scala/integration/kafka/admin/RemoteTopicCrudTest.scala:
##########
@@ -441,7 +441,29 @@ class RemoteTopicCrudTest extends IntegrationTestHarness {
AlterConfigOp.OpType.SET),
))
assertThrowsException(classOf[InvalidConfigurationException],
- () => admin.incrementalAlterConfigs(configs).all().get(), "Disabling remote storage feature on the topic level is not supported.")
+ () => admin.incrementalAlterConfigs(configs).all().get(), "It is invalid to disable remote storage without deleting remote data. " +
+ "If you want to keep the remote data and turn to read only, please set `remote.storage.enable=true,remote.log.copy.disable=true`. " +
+ "If you want to disable remote storage and delete all remote data, please set `remote.storage.enable=false,remote.log.delete.on.disable=true`.")
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = Array("kraft"))
+ def testUpdateTopicConfigWithDisablingRemoteStorageWithDeleteOnDisable(quorum: String): Unit = {
+ val admin = createAdminClient()
+ val topicConfig = new Properties
+ topicConfig.setProperty(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "true")
+ TestUtils.createTopicWithAdmin(admin, testTopicName, brokers, controllerServers, numPartitions, numReplicationFactor,
+ topicConfig = topicConfig)
+
+ val configs = new util.HashMap[ConfigResource, util.Collection[AlterConfigOp]]()
+ configs.put(new ConfigResource(ConfigResource.Type.TOPIC, testTopicName),
+ util.Arrays.asList(
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "false"),
+ AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_DELETE_ON_DISABLE_CONFIG, "true"),
+ AlterConfigOp.OpType.SET)
+ ))
+ verifyRemoteLogTopicConfigs(topicConfig)
Review Comment:
Excuse my inexperience but omehow there are no failures when run these tests locally using command `./gradlew core:test --tests RemoteTopicCrudTest `. Any way, I have updated the test to pass the changed configs for validation. Ran the test again and everything passes. Hopeful build/tests will succeed now
##########
core/src/test/scala/integration/kafka/admin/RemoteTopicCrudTest.scala:
##########
@@ -441,7 +441,29 @@ class RemoteTopicCrudTest extends IntegrationTestHarness {
AlterConfigOp.OpType.SET),
))
assertThrowsException(classOf[InvalidConfigurationException],
- () => admin.incrementalAlterConfigs(configs).all().get(), "Disabling remote storage feature on the topic level is not supported.")
+ () => admin.incrementalAlterConfigs(configs).all().get(), "It is invalid to disable remote storage without deleting remote data. " +
+ "If you want to keep the remote data and turn to read only, please set `remote.storage.enable=true,remote.log.copy.disable=true`. " +
+ "If you want to disable remote storage and delete all remote data, please set `remote.storage.enable=false,remote.log.delete.on.disable=true`.")
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = Array("kraft"))
+ def testUpdateTopicConfigWithDisablingRemoteStorageWithDeleteOnDisable(quorum: String): Unit = {
+ val admin = createAdminClient()
+ val topicConfig = new Properties
+ topicConfig.setProperty(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "true")
+ TestUtils.createTopicWithAdmin(admin, testTopicName, brokers, controllerServers, numPartitions, numReplicationFactor,
+ topicConfig = topicConfig)
+
+ val configs = new util.HashMap[ConfigResource, util.Collection[AlterConfigOp]]()
+ configs.put(new ConfigResource(ConfigResource.Type.TOPIC, testTopicName),
+ util.Arrays.asList(
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "false"),
+ AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_DELETE_ON_DISABLE_CONFIG, "true"),
+ AlterConfigOp.OpType.SET)
+ ))
+ verifyRemoteLogTopicConfigs(topicConfig)
Review Comment:
Excuse my inexperience but somehow there are no failures when run these tests locally using command `./gradlew core:test --tests RemoteTopicCrudTest `. Any way, I have updated the test to pass the changed configs for validation. Ran the test again and everything passes. Hopeful build/tests will succeed now
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2040587319
##########
core/src/test/scala/integration/kafka/admin/RemoteTopicCrudTest.scala:
##########
@@ -441,7 +441,36 @@ class RemoteTopicCrudTest extends IntegrationTestHarness {
AlterConfigOp.OpType.SET),
))
assertThrowsException(classOf[InvalidConfigurationException],
- () => admin.incrementalAlterConfigs(configs).all().get(), "Disabling remote storage feature on the topic level is not supported.")
+ () => admin.incrementalAlterConfigs(configs).all().get(), "It is invalid to disable remote storage without deleting remote data. " +
+ "If you want to keep the remote data and turn to read only, please set `remote.storage.enable=true,remote.log.copy.disable=true`. " +
+ "If you want to disable remote storage and delete all remote data, please set `remote.storage.enable=false,remote.log.delete.on.disable=true`.")
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = Array("kraft"))
+ def testUpdateTopicConfigWithDisablingRemoteStorageWithDeleteOnDisable(quorum: String): Unit = {
Review Comment:
@showuon, `testCreateRemoteTopicWithCopyDisabledAndDeleteOnDisable` is about creating topic. This one is about incremental config change. I think the scenario is different.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2029643749
##########
core/src/test/scala/integration/kafka/admin/RemoteTopicCrudTest.scala:
##########
@@ -441,7 +441,29 @@ class RemoteTopicCrudTest extends IntegrationTestHarness {
AlterConfigOp.OpType.SET),
))
assertThrowsException(classOf[InvalidConfigurationException],
- () => admin.incrementalAlterConfigs(configs).all().get(), "Disabling remote storage feature on the topic level is not supported.")
+ () => admin.incrementalAlterConfigs(configs).all().get(), "It is invalid to disable remote storage without deleting remote data. " +
+ "If you want to keep the remote data and turn to read only, please set `remote.storage.enable=true,remote.log.copy.disable=true`. " +
+ "If you want to disable remote storage and delete all remote data, please set `remote.storage.enable=false,remote.log.delete.on.disable=true`.")
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = Array("kraft"))
+ def testUpdateTopicConfigWithDisablingRemoteStorageWithDeleteOnDisable(quorum: String): Unit = {
+ val admin = createAdminClient()
+ val topicConfig = new Properties
+ topicConfig.setProperty(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "true")
+ TestUtils.createTopicWithAdmin(admin, testTopicName, brokers, controllerServers, numPartitions, numReplicationFactor,
+ topicConfig = topicConfig)
+
+ val configs = new util.HashMap[ConfigResource, util.Collection[AlterConfigOp]]()
+ configs.put(new ConfigResource(ConfigResource.Type.TOPIC, testTopicName),
+ util.Arrays.asList(
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "false"),
+ AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_DELETE_ON_DISABLE_CONFIG, "true"),
+ AlterConfigOp.OpType.SET)
+ ))
+ verifyRemoteLogTopicConfigs(topicConfig)
Review Comment:
Thanks for the update. However, the config is changed, so we have to set new value to topic config to pass the verification.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19071: Fix doc for remote.storage.enable [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #19345:
URL: https://github.com/apache/kafka/pull/19345#discussion_r2025863364
##########
core/src/test/scala/integration/kafka/admin/RemoteTopicCrudTest.scala:
##########
@@ -441,7 +441,29 @@ class RemoteTopicCrudTest extends IntegrationTestHarness {
AlterConfigOp.OpType.SET),
))
assertThrowsException(classOf[InvalidConfigurationException],
- () => admin.incrementalAlterConfigs(configs).all().get(), "Disabling remote storage feature on the topic level is not supported.")
+ () => admin.incrementalAlterConfigs(configs).all().get(), "It is invalid to disable remote storage without deleting remote data. " +
+ "If you want to keep the remote data and turn to read only, please set `remote.storage.enable=true,remote.log.copy.disable=true`. " +
+ "If you want to disable remote storage and delete all remote data, please set `remote.storage.enable=false,remote.log.delete.on.disable=true`.")
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = Array("kraft"))
+ def testUpdateTopicConfigWithDisablingRemoteStorageWithDeleteOnDisable(quorum: String): Unit = {
+ val admin = createAdminClient()
+ val topicConfig = new Properties
+ topicConfig.setProperty(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "true")
+ TestUtils.createTopicWithAdmin(admin, testTopicName, brokers, controllerServers, numPartitions, numReplicationFactor,
+ topicConfig = topicConfig)
+
+ val configs = new util.HashMap[ConfigResource, util.Collection[AlterConfigOp]]()
+ configs.put(new ConfigResource(ConfigResource.Type.TOPIC, testTopicName),
+ util.Arrays.asList(
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG, "false"),
+ AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry(TopicConfig.REMOTE_LOG_DELETE_ON_DISABLE_CONFIG, "true"),
+ AlterConfigOp.OpType.SET)
+ ))
+ verifyRemoteLogTopicConfigs(topicConfig)
Review Comment:
We have to use `admin.incrementalAlterConfigs(configs).all().get()` here. Or the configs is not updated to the server. Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org