Skip to content

[rdkafka] Topics created by context should contain default topic config #256

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

Closed
wants to merge 3 commits into from

Conversation

tPl0ch
Copy link
Contributor

@tPl0ch tPl0ch commented Nov 5, 2017

Currently the topics created via RdKakfaContext::createTopic do not contain the default topic configuration. This is unexpected, since the context knows about the topic config and should pass this information on to created topics.

@tPl0ch tPl0ch changed the title [rdkafka] Topics created by config should contain default config [rdkafka] Topics created by context should contain default config Nov 5, 2017
@tPl0ch tPl0ch changed the title [rdkafka] Topics created by context should contain default config [rdkafka] Topics created by context should contain default topic config Nov 5, 2017
@tPl0ch
Copy link
Contributor Author

tPl0ch commented Nov 5, 2017

Hmm, the test failure is weird. Running the Enqueue\RdKafka\Tests\RdKafkaContextTest::testTopicCreatedByContextShouldContainDefaultTopicConf case locally works fine. Not sure why it fails with Travis.

@makasim
Copy link
Member

makasim commented Nov 6, 2017

I don't think there is a need to set a default topic conf to a topic object.

According to the doc: the topic conf is optional and if it is not set the server should use the default one.

public RdKafka\Topic RdKafka::newTopic ( string $topic_name [, RdKafka\TopicConf $topic_conf = NULL ] )

topic_conf
Optional configuration that will be used instead of the default topic configuration.

I spotted one potential bug here that might help you out. The $destination->getConf() must return null if there wasn't custom options set.

@makasim
Copy link
Member

makasim commented Jan 17, 2018

Feel free to reopen if #263 hasn't solved the issue.

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