Add new module 'kafka_topics' #301

Closed
wants to merge 6 commits into
from

Projects

None yet

6 participants

@jbowes
jbowes commented Mar 8, 2015

The kafka_topics module creates new topics in Kafka
(http://kafka.apache.org/), or modifies existing ones (though only
increasing the partition count is supported in Kafka).

Topics can be operated on one at a time, or in a group, to save on
starting up the JVM for each topic to check its current state.

@jbowes jbowes Add new module 'kafka_topics'
The kafka_topics module creates new topics in Kafka
(http://kafka.apache.org/), or modifies existing ones (though only
increasing the partition count is supported in Kafka).

Topics can be operated on one at a time, or in a group, to save on
starting up the JVM for each topic to check its current state.
df75883
@bcoca bcoca commented on an outdated diff Mar 8, 2015
messaging/kafka_topics.py
@@ -0,0 +1,265 @@
+#!/usr/bin/env python
@bcoca
bcoca Mar 8, 2015 Member

shebang needs to be /usr/bin/python

@bcoca bcoca and 1 other commented on an outdated diff Mar 8, 2015
messaging/kafka_topics.py
+ - C(kafka-topics.sh) must be installed on the host.
+author: James Bowes
+options:
+ name:
+ description:
+ - The topic name to create or update
+ aliases: [topic]
+ partitions:
+ description:
+ - The number of partitions for I(name), or the default partitions for
+ I(topics) if nothing is specified.
+ replicas:
+ description:
+ - The replication factor for I(name), or the default replication factor
+ for I(topics) if nothing is specified.
+ topics:
@bcoca
bcoca Mar 8, 2015 Member

this is not the way we optimize, look at apt/yum that can take flatten a with_items into a single request

@jbowes
jbowes Mar 8, 2015

If I'm reading everything correctly, there's a special case just for package managers to do this?

For this module, would making name, topic, and topics all be aliases that could either be a single item or a list? It's quite likely the created topics will have all have different values for partition and replicas, and there's not native syntax for specifying all 3 required values in a single string like there is for apt.

@bcoca
bcoca Mar 9, 2015 Member

then i would say to always use topics and remove the other options, makes it simpler and cleaner, topics always takes a list and if they want a single one, list of one.

@jbowes
jbowes Mar 9, 2015

cool, will do!

@bcoca bcoca and 1 other commented on an outdated diff Mar 8, 2015
messaging/kafka_topics.py
+ - The number of partitions for I(name), or the default partitions for
+ I(topics) if nothing is specified.
+ replicas:
+ description:
+ - The replication factor for I(name), or the default replication factor
+ for I(topics) if nothing is specified.
+ topics:
+ description:
+ - A list of topic names to create or update (using the default
+ I(partitions) and I(replicas)), or a list of dicts containing I(name),
+ I(partitions), and I(replicas) values.
+ zookeeper:
+ description:
+ - A I(ZooKeeper) connection string, as used by I(Kafka).
+ required: true
+ path:
@bcoca
bcoca Mar 8, 2015 Member

we would prefer updating through an api and not depending on a script

@jbowes
jbowes Mar 8, 2015

Me too! Unfortunately, there isn't one. I thought about going directly to ZooKeeper, but the structures could change in the future, and kafka-topics.sh will prevent you from doing bad things (like increasing the replication factor), so calling the script seemed safer, at least until there's an API.

@bcoca
bcoca Mar 9, 2015 Member

-5 of my future possible kafka uses

@bcoca bcoca and 1 other commented on an outdated diff Mar 8, 2015
messaging/kafka_topics.py
+ topics=dict(default=None, type='list'),
+ topic=dict(default=None, aliases=["name"]),
+ partitions=dict(default=None, type="int"),
+ replicas=dict(default=None, type="int"),
+ ),
+ required_one_of=[["topic", "topics"]],
+ supports_check_mode=True,
+ )
+
+ changed = False
+
+ p = module.params
+ args = ["partitions", "replicas"]
+
+ topics = {}
+ if p["topic"]:
@bcoca
bcoca Mar 8, 2015 Member

we prefer to use module common functions for verifying required params

@jbowes
jbowes Mar 9, 2015

Do you mean the arguments to the AnsibleModule constructor, or something else? I couldn't figure out how to do what I wanted withe the constructor, but I can look again.

@bcoca
bcoca Mar 9, 2015 Member

the required_together method

@bcoca bcoca and 1 other commented on an outdated diff Mar 8, 2015
messaging/kafka_topics.py
+# You should have received a copy of the GNU General Public License
+# along with Ansible. If not, see <http://www.gnu.org/licenses/>.
+
+DOCUMENTATION = """
+---
+module: kafka_topics
+short_description: Create and modify Kafka topics.
+description:
+ - Create and modify I(Kafka) topics.
+notes:
+ - If you have multiple topics to modify, using the I(topics) option is much
+ faster, as it can execute C(kafka-topics.sh) fewer times.
+requirements:
+ - C(kafka-topics.sh) must be installed on the host.
+author: James Bowes
+options:
@bcoca
bcoca Mar 8, 2015 Member

is it possible to add state=present|absent and the ability to delete topics?

@jbowes
jbowes Mar 8, 2015

Yeah! I'll look in to what happens if you try and delete on an older kafka that doesn't support it. If things seem safe, I'll add it.

jbowes added some commits Mar 11, 2015
@jbowes jbowes Update kafka_topics for review feedback
- Remove the 'topic' option.
- Add a state option. This is applied to all topics provided.
5f76942
@jbowes jbowes Allow setting per-topic configs for kafka_topics 79a65d4
@gregdek
Contributor
gregdek commented Jun 17, 2015

Thanks for submitting this module to Ansible Extras. Apologies that it’s taken a while to get your module reviewed.

To help facilitate reviews, we’ve broadened the number of people who can approve modules for inclusion into the Extras repository. The list of official reviewers can be found here:

https://github.com/ansible/ansible-modules-extras/blob/devel/REVIEWERS.md

Our new policy is that if a new module is reviewed and approved by at least two official module reviewers, the module will be approved for inclusion. We will be asking the community of reviewers to take a look at these modules on a regular basis.

To ensure that your module has the best chance of being approved, please double-check that you adhere to the Ansible module guidelines: http://docs.ansible.com/developing_modules.html#module-checklist

@jamesbassett

Thanks for this @jbowes!

We're using Confluent (Kafka + Schema Registry + REST proxy). I was going to write a module similar to yours using the Confluent REST proxy, but sadly it doesn't support creating topics yet. So for now I'm using your module :)

Just some feedback....and sorry if this is particular to Confluent's packaged version of the Kafka CLI tools (I'll check when I get time), but it seems that if there's any exception (for example, I was setting the replication factor higher than the available brokers), then the CLI returns a return code of zero (WTF?) and prints the stacktrace to stdout. Your module happily ignores this (not really your fault) and says that everything was modified successfully.

I hacked your module to inspect the stdout of the CLI call for Exception, and to fail the module with the stacktrace.

I'll report back here when I get time to investigate with the standard Kafka CLI....

@robynbergeron
Contributor

Adding new process. We will be evaluating all new module PRs according to this process, effective immediately.

Thanks for submitting this new module to Ansible Extras! This module is now in community review, a process that is open to all Ansible users. In order for this module to be approved, it must gain the following votes:

“works_for_me”: If you have tested the module thoroughly, including testing of all of the module’s options, and if the module works for you, please add “works_for_me” in the comments.

“passes_guidelines”: If you have gone through the module guidelines and the module meets all of the requirements, please add “passes_guidelines” in the comments. Guidelines are available here: http://docs.ansible.com/developing_modules.html#module-checklist

“needs_revision”: If the module fails to work for you, or if it doesn’t meet guidelines, please add “needs_revision” in the comments with details about what needs to be fixed.

When a module has both “works_for_me” and “passes_guidelines” tags, we will promote the module for inclusion in Ansible Extras. At this point, you will be expected to maintain the module by fixing bugs and evaluating pull requests in a timely manner.

Thanks again for submitting your Ansible module!

@robynbergeron robynbergeron removed the P3 label Sep 25, 2015
@loia
Contributor
loia commented Oct 20, 2015

@jbowes I noticed an issue where trying to alter a topic's config but keeping the partitions and replicas the same will result in this error from the kafka-topics CLI -

Error while executing topic command The number of partitions for a topic can only be increased
kafka.admin.AdminOperationException: The number of partitions for a topic can only be increased
    at kafka.admin.AdminUtils$.addPartitions(AdminUtils.scala:114)
    at kafka.admin.TopicCommand$$anonfun$alterTopic$1.apply(TopicCommand.scala:119)
    at kafka.admin.TopicCommand$$anonfun$alterTopic$1.apply(TopicCommand.scala:100)
    at scala.collection.mutable.ResizableArray$class.foreach(ResizableArray.scala:59)
    at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:47)
    at kafka.admin.TopicCommand$.alterTopic(TopicCommand.scala:100)
    at kafka.admin.TopicCommand$.main(TopicCommand.scala:57)
    at kafka.admin.TopicCommand.main(TopicCommand.scala)

Example play -

- name: Create topic
  kafka_topics:
    topics: my-topic
    partitions: 3
    replicas: 3
    config: cleanup.policy=compact
    zookeeper: localhost:2181

# This will fail with the error above
- name: Alter topic
  kafka_topics:
    topics: my-topic
    partitions: 3
    replicas: 3
    config: cleanup.policy=delete
    zookeeper: localhost:2181

You probably need to omit the --partitions CLI arg if the value hasn't changed.

@gregdek
Contributor
gregdek commented Mar 29, 2016

Thanks @jbowes for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Apr 14, 2016

@jbowes A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Apr 29, 2016

@jbowes Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented May 15, 2016

@jbowes A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented May 30, 2016

@jbowes Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Jun 15, 2016

@jbowes A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Jun 30, 2016

@jbowes Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Jul 16, 2016

@jbowes A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Jul 31, 2016

@jbowes Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Aug 16, 2016

@jbowes A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Aug 31, 2016

@jbowes Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Sep 16, 2016

@jbowes A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Oct 1, 2016

@jbowes Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Oct 17, 2016

@jbowes A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented Oct 26, 2016

No activity in a year -- think we'll close this one. Thanks @jbowes, if you want to reopen let us know.

@gregdek gregdek closed this Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment