Skip to content

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

2 participants