-
Notifications
You must be signed in to change notification settings - Fork 1
fix: Pass through optional consumer settings when checking for offsets #196
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to include a test showing maybeConsumerSettings
is being used. Maybe running without necessary kafka config in the actor system config but then passing in consumer settings?
@@ -145,6 +148,30 @@ class TopicLoaderIntSpec extends IntegrationSpecBase { | |||
} | |||
} | |||
|
|||
"Kafka consumer settings" should { | |||
"Override default settings" in new TestContext { | |||
override implicit lazy val system: ActorSystem = ActorSystem("test-actor-system") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea of the consumerSettings override is so that if the ActorSystem already contains Kafka settings they will be ignored. So it might be worth keeping the default ActorSystem that already contains things like bootstrap.servers
and asserting the override is the one used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this to keep the default ActorSystem config. To prove the override config is being used we set bootstrapServers
to something invalid to trigger an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
We are trying to use the
kafka-topic-loader
to consume from multiple Kafka clusters. To do this we are providing an override forbootstrapServers
in the optionalconsumerSettings
in theloadAndRun
method (as opposed to loading from the defaultakka.conf
file), but there are places in theTopicLoader
where these optional consumer settings are being ignored (logging/checking offsets).This PR will allow the topic loader to work with multiple Kafka clusters.