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

Delete topic settings from broker if they are not present in the topic config #198

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

fpacifici
Copy link

Related with issue #197.

The idea of this PR is to make topic apply a bit more declarative by supporting deletion of settings that are not present in config.
As this could be a calamity if done by mistake, the behavior is disabled by default. A CLI argument --destructive is supported in the apply subcommand. If that flag is not set, the behavior is the same as it was before.

Example:

topicctl % ./build/topicctl apply --destructive  examples/local-cluster/topics/topic-default.yaml
{{topic-default local-cluster local-region local-env Topic that uses default (any) strategy for assigning partition brokers.
 map[] []} {3 2 0 map[cleanup.policy:delete max.message.bytes:5.54288e+06 message.timestamp.type:LogAppendTime] {in-rack  [] []} <nil>}}
[2024-06-07 16:53:35]  INFO Processing topic topic-default in config examples/local-cluster/topics/topic-default.yaml with cluster config /Users/filippopacifici/code/topicctl/examples/local-cluster/cluster.yaml
[2024-06-07 16:53:35]  INFO Starting apply for topic topic-default in environment local-env, cluster local-cluster
[2024-06-07 16:53:35]  INFO Validating configs...
[2024-06-07 16:53:35]  INFO Checking if topic already exists...
[2024-06-07 16:53:35]  INFO Updating existing topic 'topic-default'
[2024-06-07 16:53:35]  INFO Checking the existing state of the cluster, topic, and throttles...
[2024-06-07 16:53:35]  INFO All replicas are in-sync
[2024-06-07 16:53:35]  INFO Checking topic config settings...
[2024-06-07 16:53:35]  INFO Found 1 key(s) with different values:
-------------------------+----------------------+---------------------
           KEY           | CLUSTER VALUE (CURR) | CONFIG VALUE (NEW)  
-------------------------+----------------------+---------------------
  message.timestamp.type |                      | LogAppendTime       
-------------------------+----------------------+---------------------
[2024-06-07 16:53:35]  INFO Found 1 key(s) set in cluster but missing from config for deletion:
---------------+--------------------
      KEY      |   CLUSTER VALUE    
---------------+--------------------
  retention.ms | 6000000 (100 min)  
---------------+--------------------
OK to update to the new values in the topic config? (yes/no) 

Typing yes the retetion.ms setting is deleted.
If I run the command again there is no more diff to apply.

Test plan:

  • See the example above
  • Added a unit test as well

@fpacifici fpacifici requested a review from a team as a code owner June 7, 2024 23:56
@fpacifici fpacifici changed the title Deletes topic settings from broker if they are not present in the topic config Delete topic settings from broker if they are not present in the topic config Jun 8, 2024
fpacifici added a commit to getsentry/topicctl that referenced this pull request Jun 14, 2024
…tion

Same as segmentio#198
This is a PR in our fork so we can merge it in master here and start working on that while the PR on upstream gets reviewed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant