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

Sanity test for consumer.topics() and consumer.partitions_for_topic() #1829

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

Baisang
Copy link
Contributor

@Baisang Baisang commented Jun 1, 2019

This should hopefully address #1810. A really simple sanity check for the two methods (I guess it's also a sanity check for subscribe() as well)


This change is Reviewable

Copy link
Collaborator

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Thank you for this! Much appreciated.

left one comment, I'm wondering if the test can be even simpler than what you've got?

@pytest.mark.skipif(not version(), reason="No KAFKA_VERSION set")
def test_consumer_topics(kafka_broker, topic, version):
consumer = KafkaConsumer(bootstrap_servers=get_connect_str(kafka_broker))
consumer.subscribe([topic])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is subscribe() needed? Couldn't you just call topics() and partitions_for_topic() since they are both blocking for metadata? You may need to poll() at some point (even without an active subscription) in order to drive the I/O.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try that -- I do think it'd be good to add on other tests eventually that would check subscribe() and other methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeffwidman I changed the test to just have poll and removed subscribe

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do think it'd be good to add on other tests eventually that would check subscribe() and other methods

Absolutely agree. Happily welcome a PR. Just rather keep those in a separate test.

@Baisang Baisang force-pushed the baisang/partitions_for_topic_test branch from 1d896ee to 28a886c Compare June 5, 2019 05:16
@jeffwidman jeffwidman merged commit f126e5b into dpkp:master Jun 5, 2019
@jeffwidman
Copy link
Collaborator

Thanks again for this!

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