Skip to content

Conversation

haslam22
Copy link
Contributor

@haslam22 haslam22 commented Apr 24, 2023

We are trying to use the kafka-topic-loader to consume from multiple Kafka clusters. To do this we are providing an override for bootstrapServers in the optional consumerSettings in the loadAndRun method (as opposed to loading from the default akka.conf file), but there are places in the TopicLoader where these optional consumer settings are being ignored (logging/checking offsets).

This PR will allow the topic loader to work with multiple Kafka clusters.

@haslam22 haslam22 requested review from bcarter97 and r-glyde April 24, 2023 16:25
Copy link
Contributor

@r-glyde r-glyde left a 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")
Copy link
Member

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

Copy link
Contributor Author

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.

@haslam22 haslam22 requested review from r-glyde and bcarter97 April 26, 2023 15:53
@haslam22 haslam22 marked this pull request as draft April 26, 2023 16:48
@haslam22 haslam22 marked this pull request as ready for review April 27, 2023 09:28
Copy link
Member

@bcarter97 bcarter97 left a comment

Choose a reason for hiding this comment

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

lgtm

@bcarter97 bcarter97 merged commit 8fcd133 into master Apr 28, 2023
@bcarter97 bcarter97 deleted the pass-through-consumer-settings branch April 28, 2023 08:39
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.

5 participants