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 API #528

Merged
merged 7 commits into from
Jan 22, 2018
Merged

Delete Topic API #528

merged 7 commits into from
Jan 22, 2018

Conversation

0x2c7
Copy link
Contributor

@0x2c7 0x2c7 commented Jan 19, 2018

The management API group already has CreateTopic API. This pull request is to implement DeleteTopicAPI. The implementation is based on official documentation https://kafka.apache.org/protocol#The_Messages_DeleteTopics. I choose API version 0 because, in the newest version, which is 1, the only difference is throttle_time_ms field and it seems like we don't have a proper way to handle throttling. Therefore, the version 0 is safer.

@0x2c7 0x2c7 force-pushed the delete-topics-api branch from a5a1293 to 6c3c128 Compare January 19, 2018 16:55
Copy link
Contributor

@dasch dasch left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of comment!


expect(kafka.partitions_for(topic)).to eq 3
kafka.delete_topic(topic)
kafka.instance_variable_get(:@cluster).mark_as_stale!
Copy link
Contributor

Choose a reason for hiding this comment

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

This cluster should be marked as stale by Cluster#delete_topic.


expect do
kafka.partitions_for(topic)
end.to raise_error(Kafka::LeaderNotAvailable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use Client#has_topic? here instead?

@dasch dasch added this to the v0.6 milestone Jan 22, 2018
Copy link
Contributor

@dasch dasch left a comment

Choose a reason for hiding this comment

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

Thanks! ❤️

@dasch dasch merged commit 66075b6 into zendesk:master Jan 22, 2018
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.

2 participants