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

GH-194: Make KafkaEmbedded as a bean #294

Closed
wants to merge 2 commits into from

Conversation

artembilan
Copy link
Member

@artembilan artembilan commented Apr 21, 2017

Fixes: #194

To avoid concurrent calls of the shutdown hooks make an KafkaEmbedded
as a Lifecycle to let ApplicationContext to call stop() before
destroying itself

That way the Kafka servers and zookeeper and also working directory
are destroyed and removed before JMV shutdown hooks
and FileNotFoundException /tmp/kafka-3266968904825284552/replication-offset-checkpoint.tmp
should disappear

Fixes: spring-projects#194

To avoid concurrent calls of the shutdown hooks make an `KafkaEmbedded`
as a `Lifecycle` to let ApplicationContext to call `stop()` before
destroying itself

That way the Kafka servers and zookeeper and also working directory
are destroyed and removed before JMV shutdown hooks
and `FileNotFoundException /tmp/kafka-3266968904825284552/replication-offset-checkpoint.tmp`
should disappear
@garyrussell
Copy link
Contributor

Cool!

We should add some docs above here. Something like...

It is generally recommended to use the rule as a @ClassRule to avoid starting/stopping the broker between tests (and use a different topic for each test). If you are using Spring's test application context caching, you can also declare a KafkaEmbedded bean, so a single broker can be used across multiple test classes.

(plus some example configuration).

@artembilan
Copy link
Member Author

Yeah,,, I also thought about something like @EmbeddedKafka on the test class level, but since I'm not familiar with the Spring Test Framework caching magic, I'm not sure that registering bean from that level would work as expected...

Just need to get some feedback from @hoaz to go ahead!

@artembilan
Copy link
Member Author

Let's don't cherry-pick!
No issue from our side.
This feature can just be available from the 2.0 version

@garyrussell
Copy link
Contributor

Merged as 6b5c087

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