Ah, sorry if this was unclear, to clarify I don't intend to make any changes to the TopologyConfig vs StreamsConfig stuff in this KIP. I just wanted to acknowledge that it's not ideal to have some configs in StreamsConfig which only take effect when passed into certain APIs other than the KafkaStreams constructor, such as the Topology or StreamsBuilder constructor or the StreamsBuilder#build method.
In my opinion we should definitely clean this up at some point as it's a known source of confusion for users and easy to mess up by passing certain configs in at the wrong place. However, this is an existing problem and has been around for a while, from older configs like the topology.optimization to newer ones like the default.dsl.store.suppliers. So while this KIP is adding another config to the set of potentially-confusing configs, it's at least an established pattern, and I didn't want to slow this KIP down by trying to solve two problems at once. So, no TopologyConfig/StreamsConfig cleanup in this KIP. Hopefully I (or someone else) will find time for it in the future :) On Tue, Nov 19, 2024 at 3:03 AM Lucas Brutschy <lbruts...@confluent.io.invalid> wrote: > Hey, > > I'm a bit confused since the motivation promises a cleanup of > topologyconfig/streams config, but this doesn't seem to be part of this > kip. But on the whole, this change looks fine for me. > > Thanks for the KIP! > > Cheers, > Lucas, > > Sophie Blee-Goldman <sop...@responsive.dev> schrieb am Di., 19. Nov. 2024, > 00:48: > > > One more small change -- originally I proposed to only add this config to > > the TopologyConfig, and not to the StreamsConfig. However while > > implementing a POC I noticed that TopologyConfig does not have a > > constructor that accepts a plain properties or config map, and only one > > that takes in a StreamsConfig. Rather than adding a new constructor for > > TopologyConfig I think it makes sense to just add this new config to both > > StreamsConfig and TopologyConfig, as most people will generally want to > > create their TopologyConfig from a shared global StreamsConfig, rather > than > > instantiating separate sets of configs. > > > > I have made this change in the KIP so please take a look and let me know > if > > you have any concerns. Happy to discuss alternatives > > > > On Mon, Nov 18, 2024 at 3:27 PM Sophie Blee-Goldman < > sop...@responsive.dev > > > > > wrote: > > > > > Thanks Almog! That makes sense to me, I've updated the KIP so that the > > > ProcessorWrapper will extend Configurable but gave it a default no-op > > > implementation so that it's optional. > > > > > > On Mon, Nov 18, 2024 at 1:57 PM Almog Gavra <almog.ga...@gmail.com> > > wrote: > > > > > >> Thanks Sophie! This KIP will certainly make it easier to implement any > > >> kind > > >> of custom functionality across all processors in the DSL, I can > imagine > > >> quite a few use cases for this. > > >> > > >> One suggestion, we should consider including a configure() method that > > >> takes in Map<String, ?> configs, so that it can be configured based on > > >> things like application.id (e.g. for emitting custom metrics per > > >> processor). > > >> > > >> - Almog > > >> > > >> On Fri, Nov 15, 2024 at 10:16 PM Sophie Blee-Goldman < > > >> sop...@responsive.dev> > > >> wrote: > > >> > > >> > Hey all, > > >> > > > >> > We have a short KIP we'd like to propose which will allow injecting > > >> custom > > >> > code modules around the processors of Kafka Streams applications, > > >> including > > >> > DSL-built topologies. > > >> > > > >> > Please let us know if you have any thoughts or concerns > > >> > > > >> > <goog_579523372> > > >> > > > >> > > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1112%3A+allow+custom+processor+wrapping > > >> > > > >> > Cheers, > > >> > Sophie > > >> > > > >> > > > > > >
