Strongly-typed Configurations #42

felixschlegel · 2022-11-20T11:19:18Z

Closes issues: #9, #10

Motivation

This PR introduces the new ConsumerConfig and ProducerConfig that enable users to configure their KafkaConsumer/KafkaProducer using a strongly-typed struct rather than a string-based dictionary.

Approach

There are two structs called ConsumerConfig and ProducerConfig
ConsumerConfig and ProducerConfig share a lot of the same configuration properties, thus I introduced a protocol ClientConfig that uses protocol extension to mimic an abstract class that takes care of all common config properties
both configs contain a dictionary keeping track of all configuration string values that are required by librdkafka

Note: These new configuration types are currently translated to the legacy configuration type.
Replacing the legacy configuration type with the new configuration types will happen in another PR.

TODO

DocC Documentation
New topic configuration struct
Tests for new configs (are values being set correctly)
Double-check if there are missing/redundant options (maybe only use official Kafka config options?)

Open Questions

Do we want to support OAuth with our strongly typed configs?
Do we want to support setting SSL PEM keys in our strongly-typed configs?

FranzBusch

Back from vacation and had a chance to look at the PR finally. In general, this goes into the right direction but I think we should model the API slightly differently.

FranzBusch · 2022-12-05T11:15:08Z

Sources/SwiftKafka/New/ClientConfig.swift

+// TODO: Topic config -> see KafkaConfig in SwiftKafka
+// TODO: test that values get set accordingly
+// TODO: remove old config tests
+public protocol ClientConfig: Hashable, Equatable {


I don't think this should be a protocol. We should rather use a struct here and hide the whole dictionary thing internally. Using a protocol for this would require us to either make the consumers generic or use an existential and both are not really great.

Furthermore, what I would recommend is that we create a separate ConsumerConfig and ProducerConfig like you did and then have an internal method on them that can convert them to librdkafka config.

Modifications: * created a protocol `ClientConfig` that defines all common configuration properties * created two interface implementations, `ConsumerConfig` and `ProducerConfig`

Modifications: * `ClientConfig` protocol with protocol extension for properties used in both consumer and producer * `ConsumerConfig` and `ProducerConfig` structs * added initializer to `KafkaConfig` that allows conversion from `ClientConfig` to `KafkaConfig`

…properties contained in `ProducerConfig` and `ConsumerConfig`

Modifications: * created a new `struct TopicConfig` containing strongly typed configurations options * enable initialisation of `KafkaTopicConfig` through a given `TopicConfig` * add new `TopicConfig` to new `init` of `KafkaProducer`

Modifications: * moved common typed getters for string dictionary into a Dictionary extension * removed redundant `getString` dictionary getter * added new protocol `StringDictionaryRepresentable` that all new configs conform to

Modifications: * added DocC documentation for `TopicConfig` * added DocC documentation for common client conifguration properties * added DocC documentation for producer-specific conifguration properties * added DocC documentation for consumer-specific conifguration properties * remove "enable.partition.eof" property from consumer config as it is rdkafka internal and should not be exposed in the public API * added DocC documentation to `ConfigEnums`

Modifications: * removed TODOs * swiftformat

felixschlegel · 2022-12-21T13:17:48Z

Hey @FranzBusch,

I have implemented some changes since we last talked and would like to hear your feedback 😄 :

a new struct TopicConfiguration, similar to ConsumerConfig and ProducerConfig
DocC documentation (mostly copy-pasted from the official librdkafka config documentation)
removed the ClientConfig and duplicated the common config options. However, introduced a new protocol StringDictionaryRepresentable for types that have a dictionary representation (not used as a type, but rather for uniformity)
created a follow-up task for refactoring the package into using this new configuration API: Integration of new Configuration API into project #43

There are still some things I am unsure about:

I would like to test that everything gets set correctly. Should I write a test testing that each value is set correctly, or is that too redundant?
Currently, we expose more or less all librdkafka configurations — do we want to narrow it down to the most basic configurations and expand this on user request / provide an unsafe API that allows pro-users to set arbitrary key-value options?
librdkafka exposes configuration options regarding OAuth and SSL keys etc. — I have not tested this — does our package even intend to expose that as well?

I hope that I have explained my questions in enough detail. Please contact me if anything is unclear!

Happy Holidays 🎅🏼

Felix

FranzBusch

Thanks for the work. I think this looks quite good. I have some open concerns around some of the public types but let's move forward and reevaluate the public types at another point in time.

FranzBusch · 2023-01-20T12:27:17Z

Sources/SwiftKafka/New/ConsumerConfig.swift

+
+    /// Client group session and failure detection timeout. The consumer sends periodic heartbeats (heartbeat.interval.ms) to indicate its liveness to the broker. If no hearts are received by the broker for a group member within the session timeout, the broker will remove the consumer from the group and trigger a rebalance. The allowed range is configured with the broker configuration properties group.min.session.timeout.ms and group.max.session.timeout.ms. Also see max.poll.interval.ms.
+    public var sessionTimeoutMs: UInt {
+        get { self.dictionary.getUInt("session.timeout.ms") ?? 45000 }


Are all the default values here returned the ones that librdkafka uses?

Yes! I used librdkafka/CONFIGURATION.md for reference.

FranzBusch · 2023-01-20T12:32:04Z

I would like to test that everything gets set correctly. Should I write a test testing that each value is set correctly, or is that too redundant?
Currently, we expose more or less all librdkafka configurations — do we want to narrow it down to the most basic configurations and expand this on user request / provide an unsafe API that allows pro-users to set arbitrary key-value options?
librdkafka exposes configuration options regarding OAuth and SSL keys etc. — I have not tested this — does our package even intend to expose that as well?

Let's put all these open questions into GH issues for now

felixschlegel · 2023-01-22T10:26:37Z

I would like to test that everything gets set correctly. Should I write a test testing that each value is set correctly, or is that too redundant?
Currently, we expose more or less all librdkafka configurations — do we want to narrow it down to the most basic configurations and expand this on user request / provide an unsafe API that allows pro-users to set arbitrary key-value options?
librdkafka exposes configuration options regarding OAuth and SSL keys etc. — I have not tested this — does our package even intend to expose that as well?

Let's put all these open questions into GH issues for now

Done!

For reference: #45 , #46

kylebrowning · 2023-03-14T17:11:29Z

@felixschlegel is this ready to go?

FranzBusch · 2023-03-14T20:05:02Z

@kylebrowning yep it was just waiting on me to merge it

kylebrowning · 2023-03-14T20:05:43Z

Cool thanks, Im going to start using this in a related APNS project :)

FranzBusch reviewed Dec 5, 2022

View reviewed changes

felixschlegel added 4 commits December 5, 2022 12:33

Created structs for new ClientConfigs

9f62dce

Modifications: * created a protocol `ClientConfig` that defines all common configuration properties * created two interface implementations, `ConsumerConfig` and `ProducerConfig`

Moved configuration enums to dedicated type ConfigEnums

f3a2605

Moved AutoOffsetReset to ConsumerConfig (only used there)

c1a14d0

felixschlegel force-pushed the fs-kafka-typed-config branch from 7f09356 to c1a14d0 Compare December 5, 2022 11:38

felixschlegel added 5 commits December 20, 2022 10:32

Remove ClientConfig protocol and hard-code duplicate configuration …

638b61f

…properties contained in `ProducerConfig` and `ConsumerConfig`

Change access-level of XConfig.properties to internal

327b45d

Strongly-typed TopicConfig

bc9a7d0

Modifications: * created a new `struct TopicConfig` containing strongly typed configurations options * enable initialisation of `KafkaTopicConfig` through a given `TopicConfig` * add new `TopicConfig` to new `init` of `KafkaProducer`

Refactored Configuration String Dictionary

b7fac0c

Modifications: * moved common typed getters for string dictionary into a Dictionary extension * removed redundant `getString` dictionary getter * added new protocol `StringDictionaryRepresentable` that all new configs conform to

felixschlegel mentioned this pull request Dec 21, 2022

Integration of new Configuration API into project #43

Closed

Cleanup

c9d4224

Modifications: * removed TODOs * swiftformat

felixschlegel requested a review from FranzBusch December 21, 2022 13:17

Fix Typo

0786d85

FranzBusch approved these changes Jan 20, 2023

View reviewed changes

FranzBusch merged commit cd0dc2e into swift-server:main Mar 14, 2023

felixschlegel mentioned this pull request Jul 18, 2023

New Config: de-duplicate setters/getters properties #90

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strongly-typed Configurations #42

Strongly-typed Configurations #42

felixschlegel commented Nov 20, 2022 •

edited

Loading

FranzBusch left a comment

FranzBusch Dec 5, 2022

FranzBusch Dec 5, 2022

felixschlegel commented Dec 21, 2022 •

edited

Loading

FranzBusch left a comment

FranzBusch Jan 20, 2023

felixschlegel Jan 22, 2023

FranzBusch commented Jan 20, 2023

felixschlegel commented Jan 22, 2023

kylebrowning commented Mar 14, 2023

FranzBusch commented Mar 14, 2023

kylebrowning commented Mar 14, 2023

Strongly-typed Configurations #42

Strongly-typed Configurations #42

Conversation

felixschlegel commented Nov 20, 2022 • edited Loading

Motivation

Approach

TODO

Open Questions

FranzBusch left a comment

Choose a reason for hiding this comment

FranzBusch Dec 5, 2022

Choose a reason for hiding this comment

FranzBusch Dec 5, 2022

Choose a reason for hiding this comment

felixschlegel commented Dec 21, 2022 • edited Loading

FranzBusch left a comment

Choose a reason for hiding this comment

FranzBusch Jan 20, 2023

Choose a reason for hiding this comment

felixschlegel Jan 22, 2023

Choose a reason for hiding this comment

FranzBusch commented Jan 20, 2023

felixschlegel commented Jan 22, 2023

kylebrowning commented Mar 14, 2023

FranzBusch commented Mar 14, 2023

kylebrowning commented Mar 14, 2023

felixschlegel commented Nov 20, 2022 •

edited

Loading

felixschlegel commented Dec 21, 2022 •

edited

Loading