Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fail when trying to disable remote storage on an enabled topic #44

Draft
wants to merge 1 commit into
base: 3.6.0-2023-11-09
Choose a base branch
from

Conversation

jeqo
Copy link

@jeqo jeqo commented Nov 23, 2023

Able to fail-fast and avoid updating unexpected config:

demo main !2 ?3 > $KAFKA_HOME/bin/kafka-configs.sh --bootstrap-server localhost:9092 --entity-type topics --entity-name t2 --alter --add-config remote.storage.enable=false
Error while executing config command with args '--bootstrap-server localhost:9092 --entity-type topics --entity-name t2 --alter --add-config remote.storage.enable=false'
java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.InvalidConfigurationException: Disabling remote log on the topic is not supported.
        at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
        at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096)
        at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:180)
        at kafka.admin.ConfigCommand$.alterConfig(ConfigCommand.scala:359)
        at kafka.admin.ConfigCommand$.processCommand(ConfigCommand.scala:326)
        at kafka.admin.ConfigCommand$.main(ConfigCommand.scala:97)
        at kafka.admin.ConfigCommand.main(ConfigCommand.scala)
Caused by: org.apache.kafka.common.errors.InvalidConfigurationException: Disabling remote log on the topic is not supported.

demo main !2 ?3 > $KAFKA_HOME/bin/kafka-configs.sh --bootstrap-server localhost:9092 --entity-type topics --entity-name t2 --describe
Dynamic configs for topic t2 are:
  remote.storage.enable=true sensitive=false synonyms={DYNAMIC_TOPIC_CONFIG:remote.storage.enable=true}
  segment.bytes=1048576 sensitive=false synonyms={DYNAMIC_TOPIC_CONFIG:segment.bytes=1048576, DEFAULT_CONFIG:log.segment.bytes=1073741824}
  retention.ms=-1 sensitive=false synonyms={DYNAMIC_TOPIC_CONFIG:retention.ms=-1}
  local.retention.bytes=1 sensitive=false synonyms={DYNAMIC_TOPIC_CONFIG:local.retention.bytes=1, DEFAULT_CONFIG:log.local.retention.bytes=-2}
  retention.bytes=104857600 sensitive=false synonyms={DYNAMIC_TOPIC_CONFIG:retention.bytes=104857600, DEFAULT_CONFIG:log.retention.bytes=-1}

@@ -70,10 +70,19 @@ class TopicConfigHandler(private val replicaManager: ReplicaManager,
val logs = logManager.logsByTopic(topic)
val wasRemoteLogEnabledBeforeUpdate = logs.exists(_.remoteLogEnabled())

maybeFailIfDisablingRemoteStorage(props, wasRemoteLogEnabledBeforeUpdate)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this exception is triggered, for some reason the config is still updated; so I decided to trigger the exception earlier in the process on AdminZkClient

@jeqo jeqo force-pushed the jeqo/fix-fail-on-disable-topic-remote-storage branch from df03ad9 to 0563e25 Compare November 24, 2023 14:39
Copy link

This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch)

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

@github-actions github-actions bot added the Stale label Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant