Topic Configuration #19
Conversation
OneCricketeer
changed the title
src/main/java/io/dropwizard/kafka/lifecycle/TopicCreationLifecycleListener.java
Outdated
Show resolved
Hide resolved
src/main/resources/META-INF/services/io.dropwizard.jackson.Discoverable
Outdated
Show resolved
Hide resolved
|
@Cricket007 any update on this? |
|
Sorry, was on vacation. Addressed the review feedback. Not sure how to get the YAMLConfigurationFactory to test a list of topics, but I can do without that. Regarding the actual creation of topics, I can't seem to get |
src/test/java/io/dropwizard/kafka/integration/AdminClientIntegrationConfiguration.java
Outdated
Show resolved
Hide resolved
|
Regarding the stacktrace you showed, that's actually a different error than I have, but searching that suggests that since the spring-kafka Kafka dependency version is lower than the other that is actually included, there would be conflicts. To resolve that, I've moved kafka declaration into dependencyManagement to enforce the newer version, and now I seem to be able to get past that.
Also, you may want TravisCI to run |
src/main/java/io/dropwizard/kafka/managed/KafkaAdminClientManager.java
Outdated
Show resolved
Hide resolved
| // do nothing | ||
| if (this.topics != null && !this.topics.isEmpty()) { | ||
| // TODO: Check if topics already exist | ||
| this.adminClient.createTopics(this.topics); |
There was a problem hiding this comment.
Out of curiosity, what is the behavior of the client if createTopics is created every time the app starts up? Will it be a no-op if the topics already exist? Otherwise, the check might be pretty important.
There was a problem hiding this comment.
@Cricket007 Should the AdminClientFactory have the Jackson polymorphic deserialization type stuff set up? That way, a user could come along and add a new type of AdminClientFactory other than the basic one more simply.
|
@Cricket007 hey, let me know if you’re going to circle back to this today or tomorrow. If not I’ll cut a release for some other stuff, and release this when you have time to circle back. |
Seems to be out of scope of this feature, no? Besides, not sure what that would look like. In any case, I addressed all the other comments and wrote the simplest thing I could find for "check if topics exist" - https://stackoverflow.com/a/53442463/2308683 |
|
@Cricket007 You could argue it’s out of scope for this PR, but it’s absolutely something that we need to support. Either you or I can add that in a separate PR. |
| this.topics.removeIf(newTopic -> { | ||
| boolean match = newTopic.name().equals(t); | ||
| if (match) { | ||
| log.warn("Not attempting to re-create existing topic {}.", newTopic.name()); |
There was a problem hiding this comment.
I think maybe a debug log (or a single info log with all of the skipped topics appended together) would be more appropriate. Otherwise, every time someone restarts their app, they may be flooded with warn logs.
There was a problem hiding this comment.
Thanks again @Cricket007. I’ll add in the polymorphic deserialization bits and cut a release today or tomorrow.
NewTopicKafka objectMake topic factory discoverable(?)