Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Sophie Blee-Goldman <so...@responsive.dev>]

Thanks for the KIP! The proposal here sounds like the right direction to go
in for me, but I'll be interested to hear what others think.

I'm wondering if we should offer additional StreamsBuilder & Topology
constructors that accept a plain config map (or Properties) alongside the
StreamsConfig constructors. I'd advocate for providing only the
Map/Properties variety except that I'm pretty sure the StreamsConfig
options are needed for dependency injection and were requested by users of
Spring

Also, just to list out some alternatives, we could go in a different
direction completely by making TopologyConfig a true config in its own
right, and moving all the topology-specific configs from StreamsConfig to
TopologyConfig (ie removing them from StreamsConfig completely)

On the one hand, it might make more sense to keep configs separated and
group them by where they need to be applied. On the other hand I wouldn't
want to start proliferating config classes since I think this makes
discovery more difficult and increases the API surface area. So personally,
I think it's sufficient to just add the check and warning log described in
the KIP

We could also deprecate the default constructor for TopologyStreamsBuilder
to force people to pass configs in. I'm honestly fine either way, as I
don't really think it's much of a burden to ask users to pass configs into
one more place, but since the new check should help prevent users from
accidentally forgetting to pass the topology-specific configs into the
Topology/StreamsBuilder, then deprecating the default constructors seem
like na unnecessary step.

Sébastien -- My only other feedback for you on the KIP would be to list
these under the Rejected Alternatives section so we know what the range of
options are.

Thanks!
-Sophie

On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
sebastien.viale@michelin.com> wrote:

> Hi Everyone,
>
> I would like to start a discussion on KIP-1138: Clean up TopologyConfig
> and API for supplying configs needed by the topology<
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> >
>
> This proposal aims to simplify Kafka Streams configuration by unifying
> APIs and ensuring topology-specific configs (e.g., topology.optimization,
> processor.wrapper.class) are correctly applied to prevent silent
> misconfigurations.
>
> Regards,
>
> Sébastien
>
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Bill Bejeck <bb...@apache.org>]

Hi Sebastien,

A constructor for Map and Properties each looks good to me.
Thanks for the update to the KIP.

-Bill

On Wed, Jun 11, 2025 at 8:16 AM Lucas Brutschy
<lb...@confluent.io.invalid> wrote:

> Hi Sebastien,
>
> having both Map/Properties sounds good to me.
>
> In the example in under `New way to pass topology-specific config:`,
> you still seem to use the `StreamConfig` constructor. Could you update
> the example as well?
>
> Cheers,
> Lucas
>
> On Fri, May 23, 2025 at 3:32 PM Sebastien Viale
> <se...@michelin.com> wrote:
> >
> > Hi all,
> >
> > I’d like to reopen the discussion (I’ve been really busy over the past
> two months)
> >
> > I’ve updated the KIP to include two constructors: one accepting a Map
> and one accepting Properties.
> >
> > Does that look ok for everyone ?
> >
> > Thanks
> >
> > Sébastien
> >
> >
> > De : Sebastien Viale <se...@michelin.com>
> > Date : jeudi, 3 avril 2025 à 14:11
> > À : dev <de...@kafka.apache.org>
> > Objet : Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for
> supplying configs needed by the topology
> > Thanks, everyone, and thanks, Sophie, for this great summary.
> > Indeed, the KafkaStreams constructor uses a Properties object, so it
> might be more concise to use it.
> > Even if it is overkill, some existing applications may already use a
> Properties, while others use a Map. Personally, I think it is reasonable to
> propose both constructors. However, I wouldn’t be upset if we kept only one
> of them.
> > I will wait for final thoughts before updating the KIP with the new
> constructors.
> > Cheers !
> >
> > Sébastien
> >
> > De : Sophie Blee-Goldman <so...@responsive.dev>
> > Date : jeudi, 3 avril 2025 à 11:33
> > À : dev@kafka.apache.org <de...@kafka.apache.org>
> > Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API
> for supplying configs needed by the topology
> > Warning External sender Do not click on any links or open any
> attachments unless you trust the sender and know the content is safe.
> >
> > Sounds like we are in agreement and can move forward with this KIP.
> >
> > To sum, instead of having the new StreamsBuilder and Topology
> constructors
> > take a StreamsConfig parameter, they should take a Map or Properties
> > instead.
> >
> > Between the two of those I don't personally have much of a preference,
> and
> > some clients (like KafkaConsumer) even seem to offer both a Map and a
> > Properties version of the constructor. Imho this is a bit of an overkill
> > and choosing one or the other should be fine for us. I'll note that the
> the
> > KafkaStreams constructor uses Properties so perhaps that's the
> appropriate
> > choice here (even if I personally like the plain Map option slightly
> better)
> >
> > Any final thoughts?
> > This email was screened for spam and malicious content but exercise
> caution anyway.
> > This email was screened for spam and malicious content but exercise
> caution anyway.
> >
> >
> >
> >
> >
> > On Thu, Mar 27, 2025 at 9:17 AM Bill Bejeck <bb...@gmail.com> wrote:
> >
> > > Hi All,
> > >
> > > I also agree with the having consistency with the other clients, so I'm
> > > also a +1 for using Properties/Map.
> > >
> > > Thanks,
> > > Bill
> > >
> > > On Thu, Mar 27, 2025 at 7:28 AM Bruno Cadonna <ca...@apache.org>
> wrote:
> > >
> > > > Hi,
> > > >
> > > > I proposed to use constructors with StreamsConfig because I wanted a
> > > > public API that says to the users: "The config you pass to Streams
> will
> > > > not be modified." That the first call in a constructor is a
> > > > transformation from Properties/Map to StreamsConfig a user might or
> > > > might not know.
> > > >
> > > > I looked a bit around and found KIP-245[1] that deprecated
> KafkaStreams
> > > > constructors that take a StreamsConfig. The main motivation was that
> > > > those constructors add boilerplate because you need to construct an
> > > > additional object. Fair enough!
> > > >
> > > > Apparently, we also allow to modify the config in constructors[2].
> > > >
> > > > So the immutability property is not even possible without making
> larger
> > > > changes.
> > > >
> > > > I agree that we should keep it consistent with the other clients and
> I
> > > > also see the simplicity argument. So I am fine with just using
> > > > Properties/Map.
> > > >
> > > > Would be interesting to understand what the problem with the
> dependency
> > > > injection is. In the end a StreamsConfig is constructed from a Map.
> > > >
> > > >
> > > > Best,
> > > > Bruno
> > > >
> > > >
> > > > [1]
> > > >
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor
> ><
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor
> >>
> > > > [2]
> > > >
> > > >
> > >
> https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600
> <
> https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600
> ><
> https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600
> <
> https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600
> >>
> > > >
> > > > On 26.03.25 22:33, Sophie Blee-Goldman wrote:
> > > > > I think Lucas makes a good point. If we want to maintain
> consistency
> > > with
> > > > > the currents state and intended direction of the clients w.r.t
> config
> > > > > classes, we should definitely (and perhaps only) offer the
> > > Map/Properties
> > > > > version of the APIs
> > > > >
> > > > > I even submitted a KIP a while back to open up the visibility of
> one of
> > > > the
> > > > > ProducerConfig constructors, just to bring it in line with the
> other
> > > > > config classes (and to help shut off excessive logging), and it was
> > > > pretty
> > > > > much stopped dead by core/clients people who didn't like the
> current
> > > > > openness of these config classes and wanted to move in the opposite
> > > > > direction rather than make anything more visible (Even if just to
> bring
> > > > one
> > > > > of them to parity with the other classes)
> > > > >
> > > > > I think this is a pretty good argument for at the very least
> offering
> > > the
> > > > > plain Map/Properties version, and perhaps only offering that. IIRC
> the
> > > > only
> > > > > reason we added the StreamsConfig version of the KafkaStreams
> > > > constructors
> > > > > was to enable dependency injection for spring kafka users. I have
> to
> > > > wonder
> > > > > if there is a way to fix this in spring kafka to allow
> Map/Properties
> > > > based
> > > > > configuration, rather than forcing KafkaStreams to allow
> StreamsConfig
> > > > > constructors
> > > > >
> > > > > So as of this time I'm still personally in favor of having only the
> > > > > Map/Properties option, though that still leaves open the question
> of
> > > > which
> > > > > one (I notice that KAfkaConsumer, for example, offers both -- but
> this
> > > > > seems unnecessary and I think either Map or Properties, but not
> both,
> > > is
> > > > > sufficient)
> > > > >
> > > > > All this said I don't want to block the KIP forever on this
> question,
> > > so
> > > > I
> > > > > think we should try to wrap things up one way or the other. If we
> can't
> > > > > come to a consensus I'd advocate to just offer both a
> Map/Properties
> > > and
> > > > a
> > > > > StreamsConfig version. That way if/when we ever have to deprecate
> one
> > > of
> > > > > them, it's less disruptive to everyone who hopefully was already
> using
> > > > the
> > > > > other option (imo it's more likely we'd want/need to deprecate the
> > > > > StreamsConfig version and I also feel this will be the less popular
> > > > option
> > > > > since it seems only needed by those using spring)
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy
> > > > > <lb...@confluent.io.invalid> wrote:
> > > > >
> > > > >> Hi,
> > > > >>
> > > > >> I agree with the KIP, it makes sense to reduce the number of ways
> the
> > > > >> configs get passed. I agree with Sophie that it doesn't make
> sense to
> > > > >> rewrite the topology compilation just to delay reading the
> configs.
> > > > >>
> > > > >> The question StreamsConfig vs. Map/Properties is less clear-cut
> to me.
> > > > >> The immutability argument seems weak - if a parameter of type
> > > > >> Properties / Map is converted into a StreamsConfig copy right in
> the
> > > > >> various constructors, we'd get the same immutability benefit.
> > > > >>
> > > > >> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all
> take
> > > > >> Map<String, Object> or Properties, and explicitly restrict the
> > > > >> visibility of constructors with
> ConsumerConfig/ProducerConfig/..., and
> > > > >> KafkaStreams would decide to go a separate route, allowing only
> the
> > > > >> complex AbstractConfig class instead of the simple data type. This
> > > > >> seems pretty inconsistent for the interfaces of a single open
> source
> > > > >> project.
> > > > >>
> > > > >> Cheers,
> > > > >> Lucas
> > > > >>
> > > > >> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
> > > > >> <so...@responsive.dev> wrote:
> > > > >>>
> > > > >>> I don't believe it's possible to only pass the configurations
> into
> > > > >>> KafkaStreams, at least not without a huge refactoring of how and
> when
> > > > the
> > > > >>> topology is compiled. I looked into this for the processor
> wrapper
> > > > config
> > > > >>> since obviously it's preferable to not require configs be passed
> in
> > > to
> > > > >> the
> > > > >>> Topology/StreamsBuilder, and wasn't able to make it work. If it's
> > > > >> possible
> > > > >>> at all, it would have significantly complicated the
> already-complex
> > > > >>> topology construction code. I don't think it's worth it.
> > > > >>>
> > > > >>> If we really wanted to only pass configs into one place, I'd say
> we
> > > > >> should
> > > > >>> just require them for the Topology and StreeamsBuilder
> constructor,
> > > and
> > > > >> not
> > > > >>> pass configs into KafkaStreams at all. But that would mean a
> > > > >>> deprecation/code change affecting every single user out there,
> which
> > > > >> feels
> > > > >>> like unnecessary turmoil.
> > > > >>>
> > > > >>> As for whether to require StreamsConfig vs Map/Properties, I'm
> fine
> > > > with
> > > > >>> requiring StreamsConfig in theory, but since KafkaStreams allows
> > > users
> > > > to
> > > > >>> pass in a plain config map then some users might not instantiate
> > > > >>> StreamsConfig at all, and this would force them to do so. Maybe
> > > that's
> > > > >> not
> > > > >>> such a bad thing though...so yeah, guess I'm fine with just the
> > > > >>> StreamsConfig variant :)
> > > > >>>
> > > > >>> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org>
> > > > wrote:
> > > > >>>
> > > > >>>> Thanks for the KIP, this is great improvement.
> > > > >>>>
> > > > >>>> I'm +1 to all the comments and changes in the KIP thus far.
> > > > >>>>
> > > > >>>> I also like the proposal from Bruno regarding only passing
> > > > >> configurations
> > > > >>>> to KafkaStreams.
> > > > >>>> Regarding your comment about internal configuration propagation,
> > > would
> > > > >>>> setting the visiblitly on the Topology constructor to package
> > > private
> > > > >> or
> > > > >>>> protected solve the issue?
> > > > >>>> I'm not sure I'm addressing your concern entirely, but if we can
> > > > >> simplify
> > > > >>>> the code paths for passing configuration, I think it's worth
> some
> > > > >> tradeoffs
> > > > >>>> for the gain in simplicity.
> > > > >>>> As I'm writing this though I'm wondering if this is indeed
> possible
> > > > >> when
> > > > >>>> taking the PAPI into consideration. At any rate, I think the KIP
> > > can
> > > > >>>> proceed as is, I just wanted to add my 2cents.
> > > > >>>>
> > > > >>>> Regards,
> > > > >>>> Bill
> > > > >>>>
> > > > >>>>
> > > > >>>> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
> > > > >>>> sebastien.viale@michelin.com> wrote:
> > > > >>>>
> > > > >>>>> Hi,
> > > > >>>>>
> > > > >>>>> Thank you both for your remarks.
> > > > >>>>>
> > > > >>>>> Sophie:
> > > > >>>>>
> > > > >>>>> I have added all your proposals to the rejected alternatives.
> > > > >>>>>
> > > > >>>>> BC1:
> > > > >>>>> I agree that we should not create multiple config classes, as
> > > > >> avoiding
> > > > >>>>> that is the main goal of the KIP.
> > > > >>>>>
> > > > >>>>> It would be nice to have a single constructor (KafkaStreams)
> that
> > > > >> accepts
> > > > >>>>> the configuration. However, unless I’m mistaken, this would
> require
> > > > >>>>> handling config propagation internally to other components like
> > > > >> Topology.
> > > > >>>>>
> > > > >>>>> BC2:
> > > > >>>>> StreamsConfig is indeed immutable, and like Bruno, I believe
> this
> > > is
> > > > >> the
> > > > >>>>> safer approach.
> > > > >>>>> Best,
> > > > >>>>> Sébastien
> > > > >>>>>
> > > > >>>>>
> > > > >>>>> De : Bruno Cadonna <ca...@apache.org>
> > > > >>>>> Date : vendredi, 7 mars 2025 à 18:55
> > > > >>>>> À : dev@kafka.apache.org <de...@kafka.apache.org>
> > > > >>>>> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig
> and
> > > > >> API for
> > > > >>>>> supplying configs needed by the topology
> > > > >>>>> Warning External sender Do not click on any links or open any
> > > > >> attachments
> > > > >>>>> unless you trust the sender and know the content is safe.
> > > > >>>>>
> > > > >>>>> Hi Sébastien,
> > > > >>>>>
> > > > >>>>> Thanks for the KIP!
> > > > >>>>>
> > > > >>>>> Cleaning up the configs classes is really a good idea!
> > > > >>>>>
> > > > >>>>> BC1:
> > > > >>>>> I am in favor of having only StreamsConfig as a config class to
> > > avoid
> > > > >>>>> proliferation of config classes. However, I find it a bit ugly
> > > that a
> > > > >>>>> Streams user is required to pass the config first to the
> topology
> > > and
> > > > >>>>> then to KafkaStreams. Since we are talking about changing
> > > > >> constructors,
> > > > >>>>> I am wondering if it were possible to pass the configs only to
> > > > >>>>> KafkaStreams.
> > > > >>>>>
> > > > >>>>> BC2:
> > > > >>>>> While Sophie advocates to only pass a Map/Properties object
> into
> > > the
> > > > >>>>> constructors, I am advocating to only passing StreamsConfig
> objects
> > > > >> to
> > > > >>>>> the constructors. The reason is that Map/Properties are
> mutable and
> > > > >>>>> StreamsConfig is immutable. IMO, it would be safer to pass
> > > immutable
> > > > >>>>> configs to the constructors. But maybe there is a good reason
> to
> > > pass
> > > > >>>>> mutable configs that I do not know. Maybe Sophie can clarify.
> > > > >>>>>
> > > > >>>>> +1 to adding all considerations discussed in the thread to the
> > > > >> rejected
> > > > >>>>> alternatives.
> > > > >>>>>
> > > > >>>>>
> > > > >>>>> Best,
> > > > >>>>> Bruno
> > > > >>>>> This email was screened for spam and malicious content but
> exercise
> > > > >>>>> caution anyway.
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>> On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> > > > >>>>>> Thanks for the KIP! The proposal here sounds like the right
> > > > >> direction
> > > > >>>> to
> > > > >>>>> go
> > > > >>>>>> in for me, but I'll be interested to hear what others think.
> > > > >>>>>>
> > > > >>>>>> I'm wondering if we should offer additional StreamsBuilder &
> > > > >> Topology
> > > > >>>>>> constructors that accept a plain config map (or Properties)
> > > > >> alongside
> > > > >>>> the
> > > > >>>>>> StreamsConfig constructors. I'd advocate for providing only
> the
> > > > >>>>>> Map/Properties variety except that I'm pretty sure the
> > > > >> StreamsConfig
> > > > >>>>>> options are needed for dependency injection and were
> requested by
> > > > >> users
> > > > >>>>> of
> > > > >>>>>> Spring
> > > > >>>>>>
> > > > >>>>>> Also, just to list out some alternatives, we could go in a
> > > > >> different
> > > > >>>>>> direction completely by making TopologyConfig a true config
> in its
> > > > >> own
> > > > >>>>>> right, and moving all the topology-specific configs from
> > > > >> StreamsConfig
> > > > >>>> to
> > > > >>>>>> TopologyConfig (ie removing them from StreamsConfig
> completely)
> > > > >>>>>>
> > > > >>>>>> On the one hand, it might make more sense to keep configs
> > > > >> separated and
> > > > >>>>>> group them by where they need to be applied. On the other
> hand I
> > > > >>>> wouldn't
> > > > >>>>>> want to start proliferating config classes since I think this
> > > makes
> > > > >>>>>> discovery more difficult and increases the API surface area.
> So
> > > > >>>>> personally,
> > > > >>>>>> I think it's sufficient to just add the check and warning log
> > > > >> described
> > > > >>>>> in
> > > > >>>>>> the KIP
> > > > >>>>>>
> > > > >>>>>> We could also deprecate the default constructor for
> > > > >>>>> TopologyStreamsBuilder
> > > > >>>>>> to force people to pass configs in. I'm honestly fine either
> way,
> > > > >> as I
> > > > >>>>>> don't really think it's much of a burden to ask users to pass
> > > > >> configs
> > > > >>>>> into
> > > > >>>>>> one more place, but since the new check should help prevent
> users
> > > > >> from
> > > > >>>>>> accidentally forgetting to pass the topology-specific configs
> into
> > > > >> the
> > > > >>>>>> Topology/StreamsBuilder, then deprecating the default
> constructors
> > > > >> seem
> > > > >>>>>> like na unnecessary step.
> > > > >>>>>>
> > > > >>>>>> Sébastien -- My only other feedback for you on the KIP would
> be to
> > > > >> list
> > > > >>>>>> these under the Rejected Alternatives section so we know what
> the
> > > > >> range
> > > > >>>>> of
> > > > >>>>>> options are.
> > > > >>>>>>
> > > > >>>>>> Thanks!
> > > > >>>>>> -Sophie
> > > > >>>>>>
> > > > >>>>>> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> > > > >>>>>> sebastien.viale@michelin.com> wrote:
> > > > >>>>>>
> > > > >>>>>>> Hi Everyone,
> > > > >>>>>>>
> > > > >>>>>>> I would like to start a discussion on KIP-1138: Clean up
> > > > >>>> TopologyConfig
> > > > >>>>>>> and API for supplying configs needed by the topology<
> > > > >>>>>>>
> > > > >>>>>
> > > > >>>>
> > > > >>
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> ><
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> >>
> > > > >>>>> <
> > > > >>>>>
> > > > >>>>
> > > > >>
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> ><
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> >>
> > > > >>>>>>
> > > > >>>>>>>>
> > > > >>>>>>>
> > > > >>>>>>> This proposal aims to simplify Kafka Streams configuration by
> > > > >> unifying
> > > > >>>>>>> APIs and ensuring topology-specific configs (e.g.,
> > > > >>>>> topology.optimization,
> > > > >>>>>>> processor.wrapper.class) are correctly applied to prevent
> silent
> > > > >>>>>>> misconfigurations.
> > > > >>>>>>>
> > > > >>>>>>> Regards,
> > > > >>>>>>>
> > > > >>>>>>> Sébastien
> > > > >>>>>>>
> > > > >>>>>>>
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>
> > > > >>
> > > > >
> > > >
> > > >
> > >
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Sebastien Viale <se...@michelin.com>]

Thanks Lucas, good catch indeed, I just updated it.

Cheers
Sébastien

De : Lucas Brutschy <lb...@confluent.io.INVALID>
Date : mercredi, 11 juin 2025 à 15:09
À : dev@kafka.apache.org <de...@kafka.apache.org>
Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology
Warning External sender Do not click on any links or open any attachments unless you trust the sender and know the content is safe.

Hi Sebastien,

having both Map/Properties sounds good to me.

In the example in under `New way to pass topology-specific config:`,
you still seem to use the `StreamConfig` constructor. Could you update
the example as well?

Cheers,
Lucas
This email was screened for spam and malicious content but exercise caution anyway.



On Fri, May 23, 2025 at 3:32 PM Sebastien Viale
<se...@michelin.com> wrote:
>
> Hi all,
>
> I’d like to reopen the discussion (I’ve been really busy over the past two months)
>
> I’ve updated the KIP to include two constructors: one accepting a Map and one accepting Properties.
>
> Does that look ok for everyone ?
>
> Thanks
>
> Sébastien
>
>
> De : Sebastien Viale <se...@michelin.com>
> Date : jeudi, 3 avril 2025 à 14:11
> À : dev <de...@kafka.apache.org>
> Objet : Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology
> Thanks, everyone, and thanks, Sophie, for this great summary.
> Indeed, the KafkaStreams constructor uses a Properties object, so it might be more concise to use it.
> Even if it is overkill, some existing applications may already use a Properties, while others use a Map. Personally, I think it is reasonable to propose both constructors. However, I wouldn’t be upset if we kept only one of them.
> I will wait for final thoughts before updating the KIP with the new constructors.
> Cheers !
>
> Sébastien
>
> De : Sophie Blee-Goldman <so...@responsive.dev>
> Date : jeudi, 3 avril 2025 à 11:33
> À : dev@kafka.apache.org <de...@kafka.apache.org>
> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology
> Warning External sender Do not click on any links or open any attachments unless you trust the sender and know the content is safe.
>
> Sounds like we are in agreement and can move forward with this KIP.
>
> To sum, instead of having the new StreamsBuilder and Topology constructors
> take a StreamsConfig parameter, they should take a Map or Properties
> instead.
>
> Between the two of those I don't personally have much of a preference, and
> some clients (like KafkaConsumer) even seem to offer both a Map and a
> Properties version of the constructor. Imho this is a bit of an overkill
> and choosing one or the other should be fine for us. I'll note that the the
> KafkaStreams constructor uses Properties so perhaps that's the appropriate
> choice here (even if I personally like the plain Map option slightly better)
>
> Any final thoughts?
> This email was screened for spam and malicious content but exercise caution anyway.
> This email was screened for spam and malicious content but exercise caution anyway.
>
>
>
>
>
> On Thu, Mar 27, 2025 at 9:17 AM Bill Bejeck <bb...@gmail.com> wrote:
>
> > Hi All,
> >
> > I also agree with the having consistency with the other clients, so I'm
> > also a +1 for using Properties/Map.
> >
> > Thanks,
> > Bill
> >
> > On Thu, Mar 27, 2025 at 7:28 AM Bruno Cadonna <ca...@apache.org> wrote:
> >
> > > Hi,
> > >
> > > I proposed to use constructors with StreamsConfig because I wanted a
> > > public API that says to the users: "The config you pass to Streams will
> > > not be modified." That the first call in a constructor is a
> > > transformation from Properties/Map to StreamsConfig a user might or
> > > might not know.
> > >
> > > I looked a bit around and found KIP-245[1] that deprecated KafkaStreams
> > > constructors that take a StreamsConfig. The main motivation was that
> > > those constructors add boilerplate because you need to construct an
> > > additional object. Fair enough!
> > >
> > > Apparently, we also allow to modify the config in constructors[2].
> > >
> > > So the immutability property is not even possible without making larger
> > > changes.
> > >
> > > I agree that we should keep it consistent with the other clients and I
> > > also see the simplicity argument. So I am fine with just using
> > > Properties/Map.
> > >
> > > Would be interesting to understand what the problem with the dependency
> > > injection is. In the end a StreamsConfig is constructed from a Map.
> > >
> > >
> > > Best,
> > > Bruno
> > >
> > >
> > > [1]
> > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor><https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor>><https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor><https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor>>>
> > > [2]
> > >
> > >
> > https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600><https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600>><https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600><https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600>>>
> > >
> > > On 26.03.25 22:33, Sophie Blee-Goldman wrote:
> > > > I think Lucas makes a good point. If we want to maintain consistency
> > with
> > > > the currents state and intended direction of the clients w.r.t config
> > > > classes, we should definitely (and perhaps only) offer the
> > Map/Properties
> > > > version of the APIs
> > > >
> > > > I even submitted a KIP a while back to open up the visibility of one of
> > > the
> > > > ProducerConfig constructors, just to bring it in line with the other
> > > > config classes (and to help shut off excessive logging), and it was
> > > pretty
> > > > much stopped dead by core/clients people who didn't like the current
> > > > openness of these config classes and wanted to move in the opposite
> > > > direction rather than make anything more visible (Even if just to bring
> > > one
> > > > of them to parity with the other classes)
> > > >
> > > > I think this is a pretty good argument for at the very least offering
> > the
> > > > plain Map/Properties version, and perhaps only offering that. IIRC the
> > > only
> > > > reason we added the StreamsConfig version of the KafkaStreams
> > > constructors
> > > > was to enable dependency injection for spring kafka users. I have to
> > > wonder
> > > > if there is a way to fix this in spring kafka to allow Map/Properties
> > > based
> > > > configuration, rather than forcing KafkaStreams to allow StreamsConfig
> > > > constructors
> > > >
> > > > So as of this time I'm still personally in favor of having only the
> > > > Map/Properties option, though that still leaves open the question of
> > > which
> > > > one (I notice that KAfkaConsumer, for example, offers both -- but this
> > > > seems unnecessary and I think either Map or Properties, but not both,
> > is
> > > > sufficient)
> > > >
> > > > All this said I don't want to block the KIP forever on this question,
> > so
> > > I
> > > > think we should try to wrap things up one way or the other. If we can't
> > > > come to a consensus I'd advocate to just offer both a Map/Properties
> > and
> > > a
> > > > StreamsConfig version. That way if/when we ever have to deprecate one
> > of
> > > > them, it's less disruptive to everyone who hopefully was already using
> > > the
> > > > other option (imo it's more likely we'd want/need to deprecate the
> > > > StreamsConfig version and I also feel this will be the less popular
> > > option
> > > > since it seems only needed by those using spring)
> > > >
> > > > Thoughts?
> > > >
> > > > On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy
> > > > <lb...@confluent.io.invalid> wrote:
> > > >
> > > >> Hi,
> > > >>
> > > >> I agree with the KIP, it makes sense to reduce the number of ways the
> > > >> configs get passed. I agree with Sophie that it doesn't make sense to
> > > >> rewrite the topology compilation just to delay reading the configs.
> > > >>
> > > >> The question StreamsConfig vs. Map/Properties is less clear-cut to me.
> > > >> The immutability argument seems weak - if a parameter of type
> > > >> Properties / Map is converted into a StreamsConfig copy right in the
> > > >> various constructors, we'd get the same immutability benefit.
> > > >>
> > > >> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take
> > > >> Map<String, Object> or Properties, and explicitly restrict the
> > > >> visibility of constructors with ConsumerConfig/ProducerConfig/..., and
> > > >> KafkaStreams would decide to go a separate route, allowing only the
> > > >> complex AbstractConfig class instead of the simple data type. This
> > > >> seems pretty inconsistent for the interfaces of a single open source
> > > >> project.
> > > >>
> > > >> Cheers,
> > > >> Lucas
> > > >>
> > > >> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
> > > >> <so...@responsive.dev> wrote:
> > > >>>
> > > >>> I don't believe it's possible to only pass the configurations into
> > > >>> KafkaStreams, at least not without a huge refactoring of how and when
> > > the
> > > >>> topology is compiled. I looked into this for the processor wrapper
> > > config
> > > >>> since obviously it's preferable to not require configs be passed in
> > to
> > > >> the
> > > >>> Topology/StreamsBuilder, and wasn't able to make it work. If it's
> > > >> possible
> > > >>> at all, it would have significantly complicated the already-complex
> > > >>> topology construction code. I don't think it's worth it.
> > > >>>
> > > >>> If we really wanted to only pass configs into one place, I'd say we
> > > >> should
> > > >>> just require them for the Topology and StreeamsBuilder constructor,
> > and
> > > >> not
> > > >>> pass configs into KafkaStreams at all. But that would mean a
> > > >>> deprecation/code change affecting every single user out there, which
> > > >> feels
> > > >>> like unnecessary turmoil.
> > > >>>
> > > >>> As for whether to require StreamsConfig vs Map/Properties, I'm fine
> > > with
> > > >>> requiring StreamsConfig in theory, but since KafkaStreams allows
> > users
> > > to
> > > >>> pass in a plain config map then some users might not instantiate
> > > >>> StreamsConfig at all, and this would force them to do so. Maybe
> > that's
> > > >> not
> > > >>> such a bad thing though...so yeah, guess I'm fine with just the
> > > >>> StreamsConfig variant :)
> > > >>>
> > > >>> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org>
> > > wrote:
> > > >>>
> > > >>>> Thanks for the KIP, this is great improvement.
> > > >>>>
> > > >>>> I'm +1 to all the comments and changes in the KIP thus far.
> > > >>>>
> > > >>>> I also like the proposal from Bruno regarding only passing
> > > >> configurations
> > > >>>> to KafkaStreams.
> > > >>>> Regarding your comment about internal configuration propagation,
> > would
> > > >>>> setting the visiblitly on the Topology constructor to package
> > private
> > > >> or
> > > >>>> protected solve the issue?
> > > >>>> I'm not sure I'm addressing your concern entirely, but if we can
> > > >> simplify
> > > >>>> the code paths for passing configuration, I think it's worth some
> > > >> tradeoffs
> > > >>>> for the gain in simplicity.
> > > >>>> As I'm writing this though I'm wondering if this is indeed possible
> > > >> when
> > > >>>> taking the PAPI into consideration. At any rate, I think the KIP
> > can
> > > >>>> proceed as is, I just wanted to add my 2cents.
> > > >>>>
> > > >>>> Regards,
> > > >>>> Bill
> > > >>>>
> > > >>>>
> > > >>>> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
> > > >>>> sebastien.viale@michelin.com> wrote:
> > > >>>>
> > > >>>>> Hi,
> > > >>>>>
> > > >>>>> Thank you both for your remarks.
> > > >>>>>
> > > >>>>> Sophie:
> > > >>>>>
> > > >>>>> I have added all your proposals to the rejected alternatives.
> > > >>>>>
> > > >>>>> BC1:
> > > >>>>> I agree that we should not create multiple config classes, as
> > > >> avoiding
> > > >>>>> that is the main goal of the KIP.
> > > >>>>>
> > > >>>>> It would be nice to have a single constructor (KafkaStreams) that
> > > >> accepts
> > > >>>>> the configuration. However, unless I’m mistaken, this would require
> > > >>>>> handling config propagation internally to other components like
> > > >> Topology.
> > > >>>>>
> > > >>>>> BC2:
> > > >>>>> StreamsConfig is indeed immutable, and like Bruno, I believe this
> > is
> > > >> the
> > > >>>>> safer approach.
> > > >>>>> Best,
> > > >>>>> Sébastien
> > > >>>>>
> > > >>>>>
> > > >>>>> De : Bruno Cadonna <ca...@apache.org>
> > > >>>>> Date : vendredi, 7 mars 2025 à 18:55
> > > >>>>> À : dev@kafka.apache.org <de...@kafka.apache.org>
> > > >>>>> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and
> > > >> API for
> > > >>>>> supplying configs needed by the topology
> > > >>>>> Warning External sender Do not click on any links or open any
> > > >> attachments
> > > >>>>> unless you trust the sender and know the content is safe.
> > > >>>>>
> > > >>>>> Hi Sébastien,
> > > >>>>>
> > > >>>>> Thanks for the KIP!
> > > >>>>>
> > > >>>>> Cleaning up the configs classes is really a good idea!
> > > >>>>>
> > > >>>>> BC1:
> > > >>>>> I am in favor of having only StreamsConfig as a config class to
> > avoid
> > > >>>>> proliferation of config classes. However, I find it a bit ugly
> > that a
> > > >>>>> Streams user is required to pass the config first to the topology
> > and
> > > >>>>> then to KafkaStreams. Since we are talking about changing
> > > >> constructors,
> > > >>>>> I am wondering if it were possible to pass the configs only to
> > > >>>>> KafkaStreams.
> > > >>>>>
> > > >>>>> BC2:
> > > >>>>> While Sophie advocates to only pass a Map/Properties object into
> > the
> > > >>>>> constructors, I am advocating to only passing StreamsConfig objects
> > > >> to
> > > >>>>> the constructors. The reason is that Map/Properties are mutable and
> > > >>>>> StreamsConfig is immutable. IMO, it would be safer to pass
> > immutable
> > > >>>>> configs to the constructors. But maybe there is a good reason to
> > pass
> > > >>>>> mutable configs that I do not know. Maybe Sophie can clarify.
> > > >>>>>
> > > >>>>> +1 to adding all considerations discussed in the thread to the
> > > >> rejected
> > > >>>>> alternatives.
> > > >>>>>
> > > >>>>>
> > > >>>>> Best,
> > > >>>>> Bruno
> > > >>>>> This email was screened for spam and malicious content but exercise
> > > >>>>> caution anyway.
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>> On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> > > >>>>>> Thanks for the KIP! The proposal here sounds like the right
> > > >> direction
> > > >>>> to
> > > >>>>> go
> > > >>>>>> in for me, but I'll be interested to hear what others think.
> > > >>>>>>
> > > >>>>>> I'm wondering if we should offer additional StreamsBuilder &
> > > >> Topology
> > > >>>>>> constructors that accept a plain config map (or Properties)
> > > >> alongside
> > > >>>> the
> > > >>>>>> StreamsConfig constructors. I'd advocate for providing only the
> > > >>>>>> Map/Properties variety except that I'm pretty sure the
> > > >> StreamsConfig
> > > >>>>>> options are needed for dependency injection and were requested by
> > > >> users
> > > >>>>> of
> > > >>>>>> Spring
> > > >>>>>>
> > > >>>>>> Also, just to list out some alternatives, we could go in a
> > > >> different
> > > >>>>>> direction completely by making TopologyConfig a true config in its
> > > >> own
> > > >>>>>> right, and moving all the topology-specific configs from
> > > >> StreamsConfig
> > > >>>> to
> > > >>>>>> TopologyConfig (ie removing them from StreamsConfig completely)
> > > >>>>>>
> > > >>>>>> On the one hand, it might make more sense to keep configs
> > > >> separated and
> > > >>>>>> group them by where they need to be applied. On the other hand I
> > > >>>> wouldn't
> > > >>>>>> want to start proliferating config classes since I think this
> > makes
> > > >>>>>> discovery more difficult and increases the API surface area. So
> > > >>>>> personally,
> > > >>>>>> I think it's sufficient to just add the check and warning log
> > > >> described
> > > >>>>> in
> > > >>>>>> the KIP
> > > >>>>>>
> > > >>>>>> We could also deprecate the default constructor for
> > > >>>>> TopologyStreamsBuilder
> > > >>>>>> to force people to pass configs in. I'm honestly fine either way,
> > > >> as I
> > > >>>>>> don't really think it's much of a burden to ask users to pass
> > > >> configs
> > > >>>>> into
> > > >>>>>> one more place, but since the new check should help prevent users
> > > >> from
> > > >>>>>> accidentally forgetting to pass the topology-specific configs into
> > > >> the
> > > >>>>>> Topology/StreamsBuilder, then deprecating the default constructors
> > > >> seem
> > > >>>>>> like na unnecessary step.
> > > >>>>>>
> > > >>>>>> Sébastien -- My only other feedback for you on the KIP would be to
> > > >> list
> > > >>>>>> these under the Rejected Alternatives section so we know what the
> > > >> range
> > > >>>>> of
> > > >>>>>> options are.
> > > >>>>>>
> > > >>>>>> Thanks!
> > > >>>>>> -Sophie
> > > >>>>>>
> > > >>>>>> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> > > >>>>>> sebastien.viale@michelin.com> wrote:
> > > >>>>>>
> > > >>>>>>> Hi Everyone,
> > > >>>>>>>
> > > >>>>>>> I would like to start a discussion on KIP-1138: Clean up
> > > >>>> TopologyConfig
> > > >>>>>>> and API for supplying configs needed by the topology<
> > > >>>>>>>
> > > >>>>>
> > > >>>>
> > > >>
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>>>
> > > >>>>> <
> > > >>>>>
> > > >>>>
> > > >>
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>>>
> > > >>>>>>
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>>>> This proposal aims to simplify Kafka Streams configuration by
> > > >> unifying
> > > >>>>>>> APIs and ensuring topology-specific configs (e.g.,
> > > >>>>> topology.optimization,
> > > >>>>>>> processor.wrapper.class) are correctly applied to prevent silent
> > > >>>>>>> misconfigurations.
> > > >>>>>>>
> > > >>>>>>> Regards,
> > > >>>>>>>
> > > >>>>>>> Sébastien
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>
> > > >>
> > > >
> > >
> > >
> >

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Lucas Brutschy <lb...@confluent.io.INVALID>]

Hi Sebastien,

having both Map/Properties sounds good to me.

In the example in under `New way to pass topology-specific config:`,
you still seem to use the `StreamConfig` constructor. Could you update
the example as well?

Cheers,
Lucas

On Fri, May 23, 2025 at 3:32 PM Sebastien Viale
<se...@michelin.com> wrote:
>
> Hi all,
>
> I’d like to reopen the discussion (I’ve been really busy over the past two months)
>
> I’ve updated the KIP to include two constructors: one accepting a Map and one accepting Properties.
>
> Does that look ok for everyone ?
>
> Thanks
>
> Sébastien
>
>
> De : Sebastien Viale <se...@michelin.com>
> Date : jeudi, 3 avril 2025 à 14:11
> À : dev <de...@kafka.apache.org>
> Objet : Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology
> Thanks, everyone, and thanks, Sophie, for this great summary.
> Indeed, the KafkaStreams constructor uses a Properties object, so it might be more concise to use it.
> Even if it is overkill, some existing applications may already use a Properties, while others use a Map. Personally, I think it is reasonable to propose both constructors. However, I wouldn’t be upset if we kept only one of them.
> I will wait for final thoughts before updating the KIP with the new constructors.
> Cheers !
>
> Sébastien
>
> De : Sophie Blee-Goldman <so...@responsive.dev>
> Date : jeudi, 3 avril 2025 à 11:33
> À : dev@kafka.apache.org <de...@kafka.apache.org>
> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology
> Warning External sender Do not click on any links or open any attachments unless you trust the sender and know the content is safe.
>
> Sounds like we are in agreement and can move forward with this KIP.
>
> To sum, instead of having the new StreamsBuilder and Topology constructors
> take a StreamsConfig parameter, they should take a Map or Properties
> instead.
>
> Between the two of those I don't personally have much of a preference, and
> some clients (like KafkaConsumer) even seem to offer both a Map and a
> Properties version of the constructor. Imho this is a bit of an overkill
> and choosing one or the other should be fine for us. I'll note that the the
> KafkaStreams constructor uses Properties so perhaps that's the appropriate
> choice here (even if I personally like the plain Map option slightly better)
>
> Any final thoughts?
> This email was screened for spam and malicious content but exercise caution anyway.
> This email was screened for spam and malicious content but exercise caution anyway.
>
>
>
>
>
> On Thu, Mar 27, 2025 at 9:17 AM Bill Bejeck <bb...@gmail.com> wrote:
>
> > Hi All,
> >
> > I also agree with the having consistency with the other clients, so I'm
> > also a +1 for using Properties/Map.
> >
> > Thanks,
> > Bill
> >
> > On Thu, Mar 27, 2025 at 7:28 AM Bruno Cadonna <ca...@apache.org> wrote:
> >
> > > Hi,
> > >
> > > I proposed to use constructors with StreamsConfig because I wanted a
> > > public API that says to the users: "The config you pass to Streams will
> > > not be modified." That the first call in a constructor is a
> > > transformation from Properties/Map to StreamsConfig a user might or
> > > might not know.
> > >
> > > I looked a bit around and found KIP-245[1] that deprecated KafkaStreams
> > > constructors that take a StreamsConfig. The main motivation was that
> > > those constructors add boilerplate because you need to construct an
> > > additional object. Fair enough!
> > >
> > > Apparently, we also allow to modify the config in constructors[2].
> > >
> > > So the immutability property is not even possible without making larger
> > > changes.
> > >
> > > I agree that we should keep it consistent with the other clients and I
> > > also see the simplicity argument. So I am fine with just using
> > > Properties/Map.
> > >
> > > Would be interesting to understand what the problem with the dependency
> > > injection is. In the end a StreamsConfig is constructed from a Map.
> > >
> > >
> > > Best,
> > > Bruno
> > >
> > >
> > > [1]
> > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor><https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor>>
> > > [2]
> > >
> > >
> > https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600><https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600>>
> > >
> > > On 26.03.25 22:33, Sophie Blee-Goldman wrote:
> > > > I think Lucas makes a good point. If we want to maintain consistency
> > with
> > > > the currents state and intended direction of the clients w.r.t config
> > > > classes, we should definitely (and perhaps only) offer the
> > Map/Properties
> > > > version of the APIs
> > > >
> > > > I even submitted a KIP a while back to open up the visibility of one of
> > > the
> > > > ProducerConfig constructors, just to bring it in line with the other
> > > > config classes (and to help shut off excessive logging), and it was
> > > pretty
> > > > much stopped dead by core/clients people who didn't like the current
> > > > openness of these config classes and wanted to move in the opposite
> > > > direction rather than make anything more visible (Even if just to bring
> > > one
> > > > of them to parity with the other classes)
> > > >
> > > > I think this is a pretty good argument for at the very least offering
> > the
> > > > plain Map/Properties version, and perhaps only offering that. IIRC the
> > > only
> > > > reason we added the StreamsConfig version of the KafkaStreams
> > > constructors
> > > > was to enable dependency injection for spring kafka users. I have to
> > > wonder
> > > > if there is a way to fix this in spring kafka to allow Map/Properties
> > > based
> > > > configuration, rather than forcing KafkaStreams to allow StreamsConfig
> > > > constructors
> > > >
> > > > So as of this time I'm still personally in favor of having only the
> > > > Map/Properties option, though that still leaves open the question of
> > > which
> > > > one (I notice that KAfkaConsumer, for example, offers both -- but this
> > > > seems unnecessary and I think either Map or Properties, but not both,
> > is
> > > > sufficient)
> > > >
> > > > All this said I don't want to block the KIP forever on this question,
> > so
> > > I
> > > > think we should try to wrap things up one way or the other. If we can't
> > > > come to a consensus I'd advocate to just offer both a Map/Properties
> > and
> > > a
> > > > StreamsConfig version. That way if/when we ever have to deprecate one
> > of
> > > > them, it's less disruptive to everyone who hopefully was already using
> > > the
> > > > other option (imo it's more likely we'd want/need to deprecate the
> > > > StreamsConfig version and I also feel this will be the less popular
> > > option
> > > > since it seems only needed by those using spring)
> > > >
> > > > Thoughts?
> > > >
> > > > On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy
> > > > <lb...@confluent.io.invalid> wrote:
> > > >
> > > >> Hi,
> > > >>
> > > >> I agree with the KIP, it makes sense to reduce the number of ways the
> > > >> configs get passed. I agree with Sophie that it doesn't make sense to
> > > >> rewrite the topology compilation just to delay reading the configs.
> > > >>
> > > >> The question StreamsConfig vs. Map/Properties is less clear-cut to me.
> > > >> The immutability argument seems weak - if a parameter of type
> > > >> Properties / Map is converted into a StreamsConfig copy right in the
> > > >> various constructors, we'd get the same immutability benefit.
> > > >>
> > > >> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take
> > > >> Map<String, Object> or Properties, and explicitly restrict the
> > > >> visibility of constructors with ConsumerConfig/ProducerConfig/..., and
> > > >> KafkaStreams would decide to go a separate route, allowing only the
> > > >> complex AbstractConfig class instead of the simple data type. This
> > > >> seems pretty inconsistent for the interfaces of a single open source
> > > >> project.
> > > >>
> > > >> Cheers,
> > > >> Lucas
> > > >>
> > > >> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
> > > >> <so...@responsive.dev> wrote:
> > > >>>
> > > >>> I don't believe it's possible to only pass the configurations into
> > > >>> KafkaStreams, at least not without a huge refactoring of how and when
> > > the
> > > >>> topology is compiled. I looked into this for the processor wrapper
> > > config
> > > >>> since obviously it's preferable to not require configs be passed in
> > to
> > > >> the
> > > >>> Topology/StreamsBuilder, and wasn't able to make it work. If it's
> > > >> possible
> > > >>> at all, it would have significantly complicated the already-complex
> > > >>> topology construction code. I don't think it's worth it.
> > > >>>
> > > >>> If we really wanted to only pass configs into one place, I'd say we
> > > >> should
> > > >>> just require them for the Topology and StreeamsBuilder constructor,
> > and
> > > >> not
> > > >>> pass configs into KafkaStreams at all. But that would mean a
> > > >>> deprecation/code change affecting every single user out there, which
> > > >> feels
> > > >>> like unnecessary turmoil.
> > > >>>
> > > >>> As for whether to require StreamsConfig vs Map/Properties, I'm fine
> > > with
> > > >>> requiring StreamsConfig in theory, but since KafkaStreams allows
> > users
> > > to
> > > >>> pass in a plain config map then some users might not instantiate
> > > >>> StreamsConfig at all, and this would force them to do so. Maybe
> > that's
> > > >> not
> > > >>> such a bad thing though...so yeah, guess I'm fine with just the
> > > >>> StreamsConfig variant :)
> > > >>>
> > > >>> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org>
> > > wrote:
> > > >>>
> > > >>>> Thanks for the KIP, this is great improvement.
> > > >>>>
> > > >>>> I'm +1 to all the comments and changes in the KIP thus far.
> > > >>>>
> > > >>>> I also like the proposal from Bruno regarding only passing
> > > >> configurations
> > > >>>> to KafkaStreams.
> > > >>>> Regarding your comment about internal configuration propagation,
> > would
> > > >>>> setting the visiblitly on the Topology constructor to package
> > private
> > > >> or
> > > >>>> protected solve the issue?
> > > >>>> I'm not sure I'm addressing your concern entirely, but if we can
> > > >> simplify
> > > >>>> the code paths for passing configuration, I think it's worth some
> > > >> tradeoffs
> > > >>>> for the gain in simplicity.
> > > >>>> As I'm writing this though I'm wondering if this is indeed possible
> > > >> when
> > > >>>> taking the PAPI into consideration. At any rate, I think the KIP
> > can
> > > >>>> proceed as is, I just wanted to add my 2cents.
> > > >>>>
> > > >>>> Regards,
> > > >>>> Bill
> > > >>>>
> > > >>>>
> > > >>>> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
> > > >>>> sebastien.viale@michelin.com> wrote:
> > > >>>>
> > > >>>>> Hi,
> > > >>>>>
> > > >>>>> Thank you both for your remarks.
> > > >>>>>
> > > >>>>> Sophie:
> > > >>>>>
> > > >>>>> I have added all your proposals to the rejected alternatives.
> > > >>>>>
> > > >>>>> BC1:
> > > >>>>> I agree that we should not create multiple config classes, as
> > > >> avoiding
> > > >>>>> that is the main goal of the KIP.
> > > >>>>>
> > > >>>>> It would be nice to have a single constructor (KafkaStreams) that
> > > >> accepts
> > > >>>>> the configuration. However, unless I’m mistaken, this would require
> > > >>>>> handling config propagation internally to other components like
> > > >> Topology.
> > > >>>>>
> > > >>>>> BC2:
> > > >>>>> StreamsConfig is indeed immutable, and like Bruno, I believe this
> > is
> > > >> the
> > > >>>>> safer approach.
> > > >>>>> Best,
> > > >>>>> Sébastien
> > > >>>>>
> > > >>>>>
> > > >>>>> De : Bruno Cadonna <ca...@apache.org>
> > > >>>>> Date : vendredi, 7 mars 2025 à 18:55
> > > >>>>> À : dev@kafka.apache.org <de...@kafka.apache.org>
> > > >>>>> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and
> > > >> API for
> > > >>>>> supplying configs needed by the topology
> > > >>>>> Warning External sender Do not click on any links or open any
> > > >> attachments
> > > >>>>> unless you trust the sender and know the content is safe.
> > > >>>>>
> > > >>>>> Hi Sébastien,
> > > >>>>>
> > > >>>>> Thanks for the KIP!
> > > >>>>>
> > > >>>>> Cleaning up the configs classes is really a good idea!
> > > >>>>>
> > > >>>>> BC1:
> > > >>>>> I am in favor of having only StreamsConfig as a config class to
> > avoid
> > > >>>>> proliferation of config classes. However, I find it a bit ugly
> > that a
> > > >>>>> Streams user is required to pass the config first to the topology
> > and
> > > >>>>> then to KafkaStreams. Since we are talking about changing
> > > >> constructors,
> > > >>>>> I am wondering if it were possible to pass the configs only to
> > > >>>>> KafkaStreams.
> > > >>>>>
> > > >>>>> BC2:
> > > >>>>> While Sophie advocates to only pass a Map/Properties object into
> > the
> > > >>>>> constructors, I am advocating to only passing StreamsConfig objects
> > > >> to
> > > >>>>> the constructors. The reason is that Map/Properties are mutable and
> > > >>>>> StreamsConfig is immutable. IMO, it would be safer to pass
> > immutable
> > > >>>>> configs to the constructors. But maybe there is a good reason to
> > pass
> > > >>>>> mutable configs that I do not know. Maybe Sophie can clarify.
> > > >>>>>
> > > >>>>> +1 to adding all considerations discussed in the thread to the
> > > >> rejected
> > > >>>>> alternatives.
> > > >>>>>
> > > >>>>>
> > > >>>>> Best,
> > > >>>>> Bruno
> > > >>>>> This email was screened for spam and malicious content but exercise
> > > >>>>> caution anyway.
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>> On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> > > >>>>>> Thanks for the KIP! The proposal here sounds like the right
> > > >> direction
> > > >>>> to
> > > >>>>> go
> > > >>>>>> in for me, but I'll be interested to hear what others think.
> > > >>>>>>
> > > >>>>>> I'm wondering if we should offer additional StreamsBuilder &
> > > >> Topology
> > > >>>>>> constructors that accept a plain config map (or Properties)
> > > >> alongside
> > > >>>> the
> > > >>>>>> StreamsConfig constructors. I'd advocate for providing only the
> > > >>>>>> Map/Properties variety except that I'm pretty sure the
> > > >> StreamsConfig
> > > >>>>>> options are needed for dependency injection and were requested by
> > > >> users
> > > >>>>> of
> > > >>>>>> Spring
> > > >>>>>>
> > > >>>>>> Also, just to list out some alternatives, we could go in a
> > > >> different
> > > >>>>>> direction completely by making TopologyConfig a true config in its
> > > >> own
> > > >>>>>> right, and moving all the topology-specific configs from
> > > >> StreamsConfig
> > > >>>> to
> > > >>>>>> TopologyConfig (ie removing them from StreamsConfig completely)
> > > >>>>>>
> > > >>>>>> On the one hand, it might make more sense to keep configs
> > > >> separated and
> > > >>>>>> group them by where they need to be applied. On the other hand I
> > > >>>> wouldn't
> > > >>>>>> want to start proliferating config classes since I think this
> > makes
> > > >>>>>> discovery more difficult and increases the API surface area. So
> > > >>>>> personally,
> > > >>>>>> I think it's sufficient to just add the check and warning log
> > > >> described
> > > >>>>> in
> > > >>>>>> the KIP
> > > >>>>>>
> > > >>>>>> We could also deprecate the default constructor for
> > > >>>>> TopologyStreamsBuilder
> > > >>>>>> to force people to pass configs in. I'm honestly fine either way,
> > > >> as I
> > > >>>>>> don't really think it's much of a burden to ask users to pass
> > > >> configs
> > > >>>>> into
> > > >>>>>> one more place, but since the new check should help prevent users
> > > >> from
> > > >>>>>> accidentally forgetting to pass the topology-specific configs into
> > > >> the
> > > >>>>>> Topology/StreamsBuilder, then deprecating the default constructors
> > > >> seem
> > > >>>>>> like na unnecessary step.
> > > >>>>>>
> > > >>>>>> Sébastien -- My only other feedback for you on the KIP would be to
> > > >> list
> > > >>>>>> these under the Rejected Alternatives section so we know what the
> > > >> range
> > > >>>>> of
> > > >>>>>> options are.
> > > >>>>>>
> > > >>>>>> Thanks!
> > > >>>>>> -Sophie
> > > >>>>>>
> > > >>>>>> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> > > >>>>>> sebastien.viale@michelin.com> wrote:
> > > >>>>>>
> > > >>>>>>> Hi Everyone,
> > > >>>>>>>
> > > >>>>>>> I would like to start a discussion on KIP-1138: Clean up
> > > >>>> TopologyConfig
> > > >>>>>>> and API for supplying configs needed by the topology<
> > > >>>>>>>
> > > >>>>>
> > > >>>>
> > > >>
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>>
> > > >>>>> <
> > > >>>>>
> > > >>>>
> > > >>
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>>
> > > >>>>>>
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>>>> This proposal aims to simplify Kafka Streams configuration by
> > > >> unifying
> > > >>>>>>> APIs and ensuring topology-specific configs (e.g.,
> > > >>>>> topology.optimization,
> > > >>>>>>> processor.wrapper.class) are correctly applied to prevent silent
> > > >>>>>>> misconfigurations.
> > > >>>>>>>
> > > >>>>>>> Regards,
> > > >>>>>>>
> > > >>>>>>> Sébastien
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>
> > > >>
> > > >
> > >
> > >
> >

[DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Sebastien Viale <se...@michelin.com>]

Hi all,

I’d like to reopen the discussion (I’ve been really busy over the past two months)

I’ve updated the KIP to include two constructors: one accepting a Map and one accepting Properties.

Does that look ok for everyone ?

Thanks

Sébastien


De : Sebastien Viale <se...@michelin.com>
Date : jeudi, 3 avril 2025 à 14:11
À : dev <de...@kafka.apache.org>
Objet : Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology
Thanks, everyone, and thanks, Sophie, for this great summary.
Indeed, the KafkaStreams constructor uses a Properties object, so it might be more concise to use it.
Even if it is overkill, some existing applications may already use a Properties, while others use a Map. Personally, I think it is reasonable to propose both constructors. However, I wouldn’t be upset if we kept only one of them.
I will wait for final thoughts before updating the KIP with the new constructors.
Cheers !

Sébastien

De : Sophie Blee-Goldman <so...@responsive.dev>
Date : jeudi, 3 avril 2025 à 11:33
À : dev@kafka.apache.org <de...@kafka.apache.org>
Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology
Warning External sender Do not click on any links or open any attachments unless you trust the sender and know the content is safe.

Sounds like we are in agreement and can move forward with this KIP.

To sum, instead of having the new StreamsBuilder and Topology constructors
take a StreamsConfig parameter, they should take a Map or Properties
instead.

Between the two of those I don't personally have much of a preference, and
some clients (like KafkaConsumer) even seem to offer both a Map and a
Properties version of the constructor. Imho this is a bit of an overkill
and choosing one or the other should be fine for us. I'll note that the the
KafkaStreams constructor uses Properties so perhaps that's the appropriate
choice here (even if I personally like the plain Map option slightly better)

Any final thoughts?
This email was screened for spam and malicious content but exercise caution anyway.
This email was screened for spam and malicious content but exercise caution anyway.





On Thu, Mar 27, 2025 at 9:17 AM Bill Bejeck <bb...@gmail.com> wrote:

> Hi All,
>
> I also agree with the having consistency with the other clients, so I'm
> also a +1 for using Properties/Map.
>
> Thanks,
> Bill
>
> On Thu, Mar 27, 2025 at 7:28 AM Bruno Cadonna <ca...@apache.org> wrote:
>
> > Hi,
> >
> > I proposed to use constructors with StreamsConfig because I wanted a
> > public API that says to the users: "The config you pass to Streams will
> > not be modified." That the first call in a constructor is a
> > transformation from Properties/Map to StreamsConfig a user might or
> > might not know.
> >
> > I looked a bit around and found KIP-245[1] that deprecated KafkaStreams
> > constructors that take a StreamsConfig. The main motivation was that
> > those constructors add boilerplate because you need to construct an
> > additional object. Fair enough!
> >
> > Apparently, we also allow to modify the config in constructors[2].
> >
> > So the immutability property is not even possible without making larger
> > changes.
> >
> > I agree that we should keep it consistent with the other clients and I
> > also see the simplicity argument. So I am fine with just using
> > Properties/Map.
> >
> > Would be interesting to understand what the problem with the dependency
> > injection is. In the end a StreamsConfig is constructed from a Map.
> >
> >
> > Best,
> > Bruno
> >
> >
> > [1]
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor><https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor>>
> > [2]
> >
> >
> https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600><https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600>>
> >
> > On 26.03.25 22:33, Sophie Blee-Goldman wrote:
> > > I think Lucas makes a good point. If we want to maintain consistency
> with
> > > the currents state and intended direction of the clients w.r.t config
> > > classes, we should definitely (and perhaps only) offer the
> Map/Properties
> > > version of the APIs
> > >
> > > I even submitted a KIP a while back to open up the visibility of one of
> > the
> > > ProducerConfig constructors, just to bring it in line with the other
> > > config classes (and to help shut off excessive logging), and it was
> > pretty
> > > much stopped dead by core/clients people who didn't like the current
> > > openness of these config classes and wanted to move in the opposite
> > > direction rather than make anything more visible (Even if just to bring
> > one
> > > of them to parity with the other classes)
> > >
> > > I think this is a pretty good argument for at the very least offering
> the
> > > plain Map/Properties version, and perhaps only offering that. IIRC the
> > only
> > > reason we added the StreamsConfig version of the KafkaStreams
> > constructors
> > > was to enable dependency injection for spring kafka users. I have to
> > wonder
> > > if there is a way to fix this in spring kafka to allow Map/Properties
> > based
> > > configuration, rather than forcing KafkaStreams to allow StreamsConfig
> > > constructors
> > >
> > > So as of this time I'm still personally in favor of having only the
> > > Map/Properties option, though that still leaves open the question of
> > which
> > > one (I notice that KAfkaConsumer, for example, offers both -- but this
> > > seems unnecessary and I think either Map or Properties, but not both,
> is
> > > sufficient)
> > >
> > > All this said I don't want to block the KIP forever on this question,
> so
> > I
> > > think we should try to wrap things up one way or the other. If we can't
> > > come to a consensus I'd advocate to just offer both a Map/Properties
> and
> > a
> > > StreamsConfig version. That way if/when we ever have to deprecate one
> of
> > > them, it's less disruptive to everyone who hopefully was already using
> > the
> > > other option (imo it's more likely we'd want/need to deprecate the
> > > StreamsConfig version and I also feel this will be the less popular
> > option
> > > since it seems only needed by those using spring)
> > >
> > > Thoughts?
> > >
> > > On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy
> > > <lb...@confluent.io.invalid> wrote:
> > >
> > >> Hi,
> > >>
> > >> I agree with the KIP, it makes sense to reduce the number of ways the
> > >> configs get passed. I agree with Sophie that it doesn't make sense to
> > >> rewrite the topology compilation just to delay reading the configs.
> > >>
> > >> The question StreamsConfig vs. Map/Properties is less clear-cut to me.
> > >> The immutability argument seems weak - if a parameter of type
> > >> Properties / Map is converted into a StreamsConfig copy right in the
> > >> various constructors, we'd get the same immutability benefit.
> > >>
> > >> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take
> > >> Map<String, Object> or Properties, and explicitly restrict the
> > >> visibility of constructors with ConsumerConfig/ProducerConfig/..., and
> > >> KafkaStreams would decide to go a separate route, allowing only the
> > >> complex AbstractConfig class instead of the simple data type. This
> > >> seems pretty inconsistent for the interfaces of a single open source
> > >> project.
> > >>
> > >> Cheers,
> > >> Lucas
> > >>
> > >> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
> > >> <so...@responsive.dev> wrote:
> > >>>
> > >>> I don't believe it's possible to only pass the configurations into
> > >>> KafkaStreams, at least not without a huge refactoring of how and when
> > the
> > >>> topology is compiled. I looked into this for the processor wrapper
> > config
> > >>> since obviously it's preferable to not require configs be passed in
> to
> > >> the
> > >>> Topology/StreamsBuilder, and wasn't able to make it work. If it's
> > >> possible
> > >>> at all, it would have significantly complicated the already-complex
> > >>> topology construction code. I don't think it's worth it.
> > >>>
> > >>> If we really wanted to only pass configs into one place, I'd say we
> > >> should
> > >>> just require them for the Topology and StreeamsBuilder constructor,
> and
> > >> not
> > >>> pass configs into KafkaStreams at all. But that would mean a
> > >>> deprecation/code change affecting every single user out there, which
> > >> feels
> > >>> like unnecessary turmoil.
> > >>>
> > >>> As for whether to require StreamsConfig vs Map/Properties, I'm fine
> > with
> > >>> requiring StreamsConfig in theory, but since KafkaStreams allows
> users
> > to
> > >>> pass in a plain config map then some users might not instantiate
> > >>> StreamsConfig at all, and this would force them to do so. Maybe
> that's
> > >> not
> > >>> such a bad thing though...so yeah, guess I'm fine with just the
> > >>> StreamsConfig variant :)
> > >>>
> > >>> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org>
> > wrote:
> > >>>
> > >>>> Thanks for the KIP, this is great improvement.
> > >>>>
> > >>>> I'm +1 to all the comments and changes in the KIP thus far.
> > >>>>
> > >>>> I also like the proposal from Bruno regarding only passing
> > >> configurations
> > >>>> to KafkaStreams.
> > >>>> Regarding your comment about internal configuration propagation,
> would
> > >>>> setting the visiblitly on the Topology constructor to package
> private
> > >> or
> > >>>> protected solve the issue?
> > >>>> I'm not sure I'm addressing your concern entirely, but if we can
> > >> simplify
> > >>>> the code paths for passing configuration, I think it's worth some
> > >> tradeoffs
> > >>>> for the gain in simplicity.
> > >>>> As I'm writing this though I'm wondering if this is indeed possible
> > >> when
> > >>>> taking the PAPI into consideration. At any rate, I think the KIP
> can
> > >>>> proceed as is, I just wanted to add my 2cents.
> > >>>>
> > >>>> Regards,
> > >>>> Bill
> > >>>>
> > >>>>
> > >>>> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
> > >>>> sebastien.viale@michelin.com> wrote:
> > >>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> Thank you both for your remarks.
> > >>>>>
> > >>>>> Sophie:
> > >>>>>
> > >>>>> I have added all your proposals to the rejected alternatives.
> > >>>>>
> > >>>>> BC1:
> > >>>>> I agree that we should not create multiple config classes, as
> > >> avoiding
> > >>>>> that is the main goal of the KIP.
> > >>>>>
> > >>>>> It would be nice to have a single constructor (KafkaStreams) that
> > >> accepts
> > >>>>> the configuration. However, unless I’m mistaken, this would require
> > >>>>> handling config propagation internally to other components like
> > >> Topology.
> > >>>>>
> > >>>>> BC2:
> > >>>>> StreamsConfig is indeed immutable, and like Bruno, I believe this
> is
> > >> the
> > >>>>> safer approach.
> > >>>>> Best,
> > >>>>> Sébastien
> > >>>>>
> > >>>>>
> > >>>>> De : Bruno Cadonna <ca...@apache.org>
> > >>>>> Date : vendredi, 7 mars 2025 à 18:55
> > >>>>> À : dev@kafka.apache.org <de...@kafka.apache.org>
> > >>>>> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and
> > >> API for
> > >>>>> supplying configs needed by the topology
> > >>>>> Warning External sender Do not click on any links or open any
> > >> attachments
> > >>>>> unless you trust the sender and know the content is safe.
> > >>>>>
> > >>>>> Hi Sébastien,
> > >>>>>
> > >>>>> Thanks for the KIP!
> > >>>>>
> > >>>>> Cleaning up the configs classes is really a good idea!
> > >>>>>
> > >>>>> BC1:
> > >>>>> I am in favor of having only StreamsConfig as a config class to
> avoid
> > >>>>> proliferation of config classes. However, I find it a bit ugly
> that a
> > >>>>> Streams user is required to pass the config first to the topology
> and
> > >>>>> then to KafkaStreams. Since we are talking about changing
> > >> constructors,
> > >>>>> I am wondering if it were possible to pass the configs only to
> > >>>>> KafkaStreams.
> > >>>>>
> > >>>>> BC2:
> > >>>>> While Sophie advocates to only pass a Map/Properties object into
> the
> > >>>>> constructors, I am advocating to only passing StreamsConfig objects
> > >> to
> > >>>>> the constructors. The reason is that Map/Properties are mutable and
> > >>>>> StreamsConfig is immutable. IMO, it would be safer to pass
> immutable
> > >>>>> configs to the constructors. But maybe there is a good reason to
> pass
> > >>>>> mutable configs that I do not know. Maybe Sophie can clarify.
> > >>>>>
> > >>>>> +1 to adding all considerations discussed in the thread to the
> > >> rejected
> > >>>>> alternatives.
> > >>>>>
> > >>>>>
> > >>>>> Best,
> > >>>>> Bruno
> > >>>>> This email was screened for spam and malicious content but exercise
> > >>>>> caution anyway.
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> > >>>>>> Thanks for the KIP! The proposal here sounds like the right
> > >> direction
> > >>>> to
> > >>>>> go
> > >>>>>> in for me, but I'll be interested to hear what others think.
> > >>>>>>
> > >>>>>> I'm wondering if we should offer additional StreamsBuilder &
> > >> Topology
> > >>>>>> constructors that accept a plain config map (or Properties)
> > >> alongside
> > >>>> the
> > >>>>>> StreamsConfig constructors. I'd advocate for providing only the
> > >>>>>> Map/Properties variety except that I'm pretty sure the
> > >> StreamsConfig
> > >>>>>> options are needed for dependency injection and were requested by
> > >> users
> > >>>>> of
> > >>>>>> Spring
> > >>>>>>
> > >>>>>> Also, just to list out some alternatives, we could go in a
> > >> different
> > >>>>>> direction completely by making TopologyConfig a true config in its
> > >> own
> > >>>>>> right, and moving all the topology-specific configs from
> > >> StreamsConfig
> > >>>> to
> > >>>>>> TopologyConfig (ie removing them from StreamsConfig completely)
> > >>>>>>
> > >>>>>> On the one hand, it might make more sense to keep configs
> > >> separated and
> > >>>>>> group them by where they need to be applied. On the other hand I
> > >>>> wouldn't
> > >>>>>> want to start proliferating config classes since I think this
> makes
> > >>>>>> discovery more difficult and increases the API surface area. So
> > >>>>> personally,
> > >>>>>> I think it's sufficient to just add the check and warning log
> > >> described
> > >>>>> in
> > >>>>>> the KIP
> > >>>>>>
> > >>>>>> We could also deprecate the default constructor for
> > >>>>> TopologyStreamsBuilder
> > >>>>>> to force people to pass configs in. I'm honestly fine either way,
> > >> as I
> > >>>>>> don't really think it's much of a burden to ask users to pass
> > >> configs
> > >>>>> into
> > >>>>>> one more place, but since the new check should help prevent users
> > >> from
> > >>>>>> accidentally forgetting to pass the topology-specific configs into
> > >> the
> > >>>>>> Topology/StreamsBuilder, then deprecating the default constructors
> > >> seem
> > >>>>>> like na unnecessary step.
> > >>>>>>
> > >>>>>> Sébastien -- My only other feedback for you on the KIP would be to
> > >> list
> > >>>>>> these under the Rejected Alternatives section so we know what the
> > >> range
> > >>>>> of
> > >>>>>> options are.
> > >>>>>>
> > >>>>>> Thanks!
> > >>>>>> -Sophie
> > >>>>>>
> > >>>>>> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> > >>>>>> sebastien.viale@michelin.com> wrote:
> > >>>>>>
> > >>>>>>> Hi Everyone,
> > >>>>>>>
> > >>>>>>> I would like to start a discussion on KIP-1138: Clean up
> > >>>> TopologyConfig
> > >>>>>>> and API for supplying configs needed by the topology<
> > >>>>>>>
> > >>>>>
> > >>>>
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>>
> > >>>>> <
> > >>>>>
> > >>>>
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>>
> > >>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> This proposal aims to simplify Kafka Streams configuration by
> > >> unifying
> > >>>>>>> APIs and ensuring topology-specific configs (e.g.,
> > >>>>> topology.optimization,
> > >>>>>>> processor.wrapper.class) are correctly applied to prevent silent
> > >>>>>>> misconfigurations.
> > >>>>>>>
> > >>>>>>> Regards,
> > >>>>>>>
> > >>>>>>> Sébastien
> > >>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>
> > >
> >
> >
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Sebastien Viale <se...@michelin.com>]

Thanks, everyone, and thanks, Sophie, for this great summary.
Indeed, the KafkaStreams constructor uses a Properties object, so it might be more concise to use it.
Even if it is overkill, some existing applications may already use a Properties, while others use a Map. Personally, I think it is reasonable to propose both constructors. However, I wouldn’t be upset if we kept only one of them.
I will wait for final thoughts before updating the KIP with the new constructors.
Cheers !

Sébastien

De : Sophie Blee-Goldman <so...@responsive.dev>
Date : jeudi, 3 avril 2025 à 11:33
À : dev@kafka.apache.org <de...@kafka.apache.org>
Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology
Warning External sender Do not click on any links or open any attachments unless you trust the sender and know the content is safe.

Sounds like we are in agreement and can move forward with this KIP.

To sum, instead of having the new StreamsBuilder and Topology constructors
take a StreamsConfig parameter, they should take a Map or Properties
instead.

Between the two of those I don't personally have much of a preference, and
some clients (like KafkaConsumer) even seem to offer both a Map and a
Properties version of the constructor. Imho this is a bit of an overkill
and choosing one or the other should be fine for us. I'll note that the the
KafkaStreams constructor uses Properties so perhaps that's the appropriate
choice here (even if I personally like the plain Map option slightly better)

Any final thoughts?
This email was screened for spam and malicious content but exercise caution anyway.



On Thu, Mar 27, 2025 at 9:17 AM Bill Bejeck <bb...@gmail.com> wrote:

> Hi All,
>
> I also agree with the having consistency with the other clients, so I'm
> also a +1 for using Properties/Map.
>
> Thanks,
> Bill
>
> On Thu, Mar 27, 2025 at 7:28 AM Bruno Cadonna <ca...@apache.org> wrote:
>
> > Hi,
> >
> > I proposed to use constructors with StreamsConfig because I wanted a
> > public API that says to the users: "The config you pass to Streams will
> > not be modified." That the first call in a constructor is a
> > transformation from Properties/Map to StreamsConfig a user might or
> > might not know.
> >
> > I looked a bit around and found KIP-245[1] that deprecated KafkaStreams
> > constructors that take a StreamsConfig. The main motivation was that
> > those constructors add boilerplate because you need to construct an
> > additional object. Fair enough!
> >
> > Apparently, we also allow to modify the config in constructors[2].
> >
> > So the immutability property is not even possible without making larger
> > changes.
> >
> > I agree that we should keep it consistent with the other clients and I
> > also see the simplicity argument. So I am fine with just using
> > Properties/Map.
> >
> > Would be interesting to understand what the problem with the dependency
> > injection is. In the end a StreamsConfig is constructed from a Map.
> >
> >
> > Best,
> > Bruno
> >
> >
> > [1]
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor>
> > [2]
> >
> >
> https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600>
> >
> > On 26.03.25 22:33, Sophie Blee-Goldman wrote:
> > > I think Lucas makes a good point. If we want to maintain consistency
> with
> > > the currents state and intended direction of the clients w.r.t config
> > > classes, we should definitely (and perhaps only) offer the
> Map/Properties
> > > version of the APIs
> > >
> > > I even submitted a KIP a while back to open up the visibility of one of
> > the
> > > ProducerConfig constructors, just to bring it in line with the other
> > > config classes (and to help shut off excessive logging), and it was
> > pretty
> > > much stopped dead by core/clients people who didn't like the current
> > > openness of these config classes and wanted to move in the opposite
> > > direction rather than make anything more visible (Even if just to bring
> > one
> > > of them to parity with the other classes)
> > >
> > > I think this is a pretty good argument for at the very least offering
> the
> > > plain Map/Properties version, and perhaps only offering that. IIRC the
> > only
> > > reason we added the StreamsConfig version of the KafkaStreams
> > constructors
> > > was to enable dependency injection for spring kafka users. I have to
> > wonder
> > > if there is a way to fix this in spring kafka to allow Map/Properties
> > based
> > > configuration, rather than forcing KafkaStreams to allow StreamsConfig
> > > constructors
> > >
> > > So as of this time I'm still personally in favor of having only the
> > > Map/Properties option, though that still leaves open the question of
> > which
> > > one (I notice that KAfkaConsumer, for example, offers both -- but this
> > > seems unnecessary and I think either Map or Properties, but not both,
> is
> > > sufficient)
> > >
> > > All this said I don't want to block the KIP forever on this question,
> so
> > I
> > > think we should try to wrap things up one way or the other. If we can't
> > > come to a consensus I'd advocate to just offer both a Map/Properties
> and
> > a
> > > StreamsConfig version. That way if/when we ever have to deprecate one
> of
> > > them, it's less disruptive to everyone who hopefully was already using
> > the
> > > other option (imo it's more likely we'd want/need to deprecate the
> > > StreamsConfig version and I also feel this will be the less popular
> > option
> > > since it seems only needed by those using spring)
> > >
> > > Thoughts?
> > >
> > > On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy
> > > <lb...@confluent.io.invalid> wrote:
> > >
> > >> Hi,
> > >>
> > >> I agree with the KIP, it makes sense to reduce the number of ways the
> > >> configs get passed. I agree with Sophie that it doesn't make sense to
> > >> rewrite the topology compilation just to delay reading the configs.
> > >>
> > >> The question StreamsConfig vs. Map/Properties is less clear-cut to me.
> > >> The immutability argument seems weak - if a parameter of type
> > >> Properties / Map is converted into a StreamsConfig copy right in the
> > >> various constructors, we'd get the same immutability benefit.
> > >>
> > >> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take
> > >> Map<String, Object> or Properties, and explicitly restrict the
> > >> visibility of constructors with ConsumerConfig/ProducerConfig/..., and
> > >> KafkaStreams would decide to go a separate route, allowing only the
> > >> complex AbstractConfig class instead of the simple data type. This
> > >> seems pretty inconsistent for the interfaces of a single open source
> > >> project.
> > >>
> > >> Cheers,
> > >> Lucas
> > >>
> > >> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
> > >> <so...@responsive.dev> wrote:
> > >>>
> > >>> I don't believe it's possible to only pass the configurations into
> > >>> KafkaStreams, at least not without a huge refactoring of how and when
> > the
> > >>> topology is compiled. I looked into this for the processor wrapper
> > config
> > >>> since obviously it's preferable to not require configs be passed in
> to
> > >> the
> > >>> Topology/StreamsBuilder, and wasn't able to make it work. If it's
> > >> possible
> > >>> at all, it would have significantly complicated the already-complex
> > >>> topology construction code. I don't think it's worth it.
> > >>>
> > >>> If we really wanted to only pass configs into one place, I'd say we
> > >> should
> > >>> just require them for the Topology and StreeamsBuilder constructor,
> and
> > >> not
> > >>> pass configs into KafkaStreams at all. But that would mean a
> > >>> deprecation/code change affecting every single user out there, which
> > >> feels
> > >>> like unnecessary turmoil.
> > >>>
> > >>> As for whether to require StreamsConfig vs Map/Properties, I'm fine
> > with
> > >>> requiring StreamsConfig in theory, but since KafkaStreams allows
> users
> > to
> > >>> pass in a plain config map then some users might not instantiate
> > >>> StreamsConfig at all, and this would force them to do so. Maybe
> that's
> > >> not
> > >>> such a bad thing though...so yeah, guess I'm fine with just the
> > >>> StreamsConfig variant :)
> > >>>
> > >>> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org>
> > wrote:
> > >>>
> > >>>> Thanks for the KIP, this is great improvement.
> > >>>>
> > >>>> I'm +1 to all the comments and changes in the KIP thus far.
> > >>>>
> > >>>> I also like the proposal from Bruno regarding only passing
> > >> configurations
> > >>>> to KafkaStreams.
> > >>>> Regarding your comment about internal configuration propagation,
> would
> > >>>> setting the visiblitly on the Topology constructor to package
> private
> > >> or
> > >>>> protected solve the issue?
> > >>>> I'm not sure I'm addressing your concern entirely, but if we can
> > >> simplify
> > >>>> the code paths for passing configuration, I think it's worth some
> > >> tradeoffs
> > >>>> for the gain in simplicity.
> > >>>> As I'm writing this though I'm wondering if this is indeed possible
> > >> when
> > >>>> taking the PAPI into consideration. At any rate, I think the KIP
> can
> > >>>> proceed as is, I just wanted to add my 2cents.
> > >>>>
> > >>>> Regards,
> > >>>> Bill
> > >>>>
> > >>>>
> > >>>> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
> > >>>> sebastien.viale@michelin.com> wrote:
> > >>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> Thank you both for your remarks.
> > >>>>>
> > >>>>> Sophie:
> > >>>>>
> > >>>>> I have added all your proposals to the rejected alternatives.
> > >>>>>
> > >>>>> BC1:
> > >>>>> I agree that we should not create multiple config classes, as
> > >> avoiding
> > >>>>> that is the main goal of the KIP.
> > >>>>>
> > >>>>> It would be nice to have a single constructor (KafkaStreams) that
> > >> accepts
> > >>>>> the configuration. However, unless I’m mistaken, this would require
> > >>>>> handling config propagation internally to other components like
> > >> Topology.
> > >>>>>
> > >>>>> BC2:
> > >>>>> StreamsConfig is indeed immutable, and like Bruno, I believe this
> is
> > >> the
> > >>>>> safer approach.
> > >>>>> Best,
> > >>>>> Sébastien
> > >>>>>
> > >>>>>
> > >>>>> De : Bruno Cadonna <ca...@apache.org>
> > >>>>> Date : vendredi, 7 mars 2025 à 18:55
> > >>>>> À : dev@kafka.apache.org <de...@kafka.apache.org>
> > >>>>> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and
> > >> API for
> > >>>>> supplying configs needed by the topology
> > >>>>> Warning External sender Do not click on any links or open any
> > >> attachments
> > >>>>> unless you trust the sender and know the content is safe.
> > >>>>>
> > >>>>> Hi Sébastien,
> > >>>>>
> > >>>>> Thanks for the KIP!
> > >>>>>
> > >>>>> Cleaning up the configs classes is really a good idea!
> > >>>>>
> > >>>>> BC1:
> > >>>>> I am in favor of having only StreamsConfig as a config class to
> avoid
> > >>>>> proliferation of config classes. However, I find it a bit ugly
> that a
> > >>>>> Streams user is required to pass the config first to the topology
> and
> > >>>>> then to KafkaStreams. Since we are talking about changing
> > >> constructors,
> > >>>>> I am wondering if it were possible to pass the configs only to
> > >>>>> KafkaStreams.
> > >>>>>
> > >>>>> BC2:
> > >>>>> While Sophie advocates to only pass a Map/Properties object into
> the
> > >>>>> constructors, I am advocating to only passing StreamsConfig objects
> > >> to
> > >>>>> the constructors. The reason is that Map/Properties are mutable and
> > >>>>> StreamsConfig is immutable. IMO, it would be safer to pass
> immutable
> > >>>>> configs to the constructors. But maybe there is a good reason to
> pass
> > >>>>> mutable configs that I do not know. Maybe Sophie can clarify.
> > >>>>>
> > >>>>> +1 to adding all considerations discussed in the thread to the
> > >> rejected
> > >>>>> alternatives.
> > >>>>>
> > >>>>>
> > >>>>> Best,
> > >>>>> Bruno
> > >>>>> This email was screened for spam and malicious content but exercise
> > >>>>> caution anyway.
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> > >>>>>> Thanks for the KIP! The proposal here sounds like the right
> > >> direction
> > >>>> to
> > >>>>> go
> > >>>>>> in for me, but I'll be interested to hear what others think.
> > >>>>>>
> > >>>>>> I'm wondering if we should offer additional StreamsBuilder &
> > >> Topology
> > >>>>>> constructors that accept a plain config map (or Properties)
> > >> alongside
> > >>>> the
> > >>>>>> StreamsConfig constructors. I'd advocate for providing only the
> > >>>>>> Map/Properties variety except that I'm pretty sure the
> > >> StreamsConfig
> > >>>>>> options are needed for dependency injection and were requested by
> > >> users
> > >>>>> of
> > >>>>>> Spring
> > >>>>>>
> > >>>>>> Also, just to list out some alternatives, we could go in a
> > >> different
> > >>>>>> direction completely by making TopologyConfig a true config in its
> > >> own
> > >>>>>> right, and moving all the topology-specific configs from
> > >> StreamsConfig
> > >>>> to
> > >>>>>> TopologyConfig (ie removing them from StreamsConfig completely)
> > >>>>>>
> > >>>>>> On the one hand, it might make more sense to keep configs
> > >> separated and
> > >>>>>> group them by where they need to be applied. On the other hand I
> > >>>> wouldn't
> > >>>>>> want to start proliferating config classes since I think this
> makes
> > >>>>>> discovery more difficult and increases the API surface area. So
> > >>>>> personally,
> > >>>>>> I think it's sufficient to just add the check and warning log
> > >> described
> > >>>>> in
> > >>>>>> the KIP
> > >>>>>>
> > >>>>>> We could also deprecate the default constructor for
> > >>>>> TopologyStreamsBuilder
> > >>>>>> to force people to pass configs in. I'm honestly fine either way,
> > >> as I
> > >>>>>> don't really think it's much of a burden to ask users to pass
> > >> configs
> > >>>>> into
> > >>>>>> one more place, but since the new check should help prevent users
> > >> from
> > >>>>>> accidentally forgetting to pass the topology-specific configs into
> > >> the
> > >>>>>> Topology/StreamsBuilder, then deprecating the default constructors
> > >> seem
> > >>>>>> like na unnecessary step.
> > >>>>>>
> > >>>>>> Sébastien -- My only other feedback for you on the KIP would be to
> > >> list
> > >>>>>> these under the Rejected Alternatives section so we know what the
> > >> range
> > >>>>> of
> > >>>>>> options are.
> > >>>>>>
> > >>>>>> Thanks!
> > >>>>>> -Sophie
> > >>>>>>
> > >>>>>> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> > >>>>>> sebastien.viale@michelin.com> wrote:
> > >>>>>>
> > >>>>>>> Hi Everyone,
> > >>>>>>>
> > >>>>>>> I would like to start a discussion on KIP-1138: Clean up
> > >>>> TopologyConfig
> > >>>>>>> and API for supplying configs needed by the topology<
> > >>>>>>>
> > >>>>>
> > >>>>
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>
> > >>>>> <
> > >>>>>
> > >>>>
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>
> > >>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> This proposal aims to simplify Kafka Streams configuration by
> > >> unifying
> > >>>>>>> APIs and ensuring topology-specific configs (e.g.,
> > >>>>> topology.optimization,
> > >>>>>>> processor.wrapper.class) are correctly applied to prevent silent
> > >>>>>>> misconfigurations.
> > >>>>>>>
> > >>>>>>> Regards,
> > >>>>>>>
> > >>>>>>> Sébastien
> > >>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>
> > >
> >
> >
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Sophie Blee-Goldman <so...@responsive.dev>]

This was nagging at me so I dug around and managed to find the original
discussion around needing StreamsConfig for dependency injection:
https://github.com/apache/kafka/pull/5344

tl;dr the StreamsConfig overloads aren't doing anything useful on their
own, we definitely shouldn't use StreamsConfig for the new constructors in
this KIP, but should we go a step further and re-deprecate the KafkaStreams
constructors that use StreamsConfig? And do we need a KIP to do so?

Slightly longer version: this is all a bit messy but basically this is what
went down:
1. That discussion is what resulted in us removing the @Deprecated
annotation from the StreamsConfig overloads of the KafkaStreams constructor
(without a KIP -- more on this later)
2. the author of that discussion proposed this KIP: KIP-378: Enable
Dependency Injection for Kafka Streams handlers
<https://cwiki.apache.org/confluence/display/KAFKA/KIP-378%3A+Enable+Dependency+Injection+for+Kafka+Streams+handlers>
3. KIP-378 was abandoned at some point but is apparently subsumed by  KIP-832:
Allow creating a producer/consumer using a producer/consumer config
<https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578>
4. KIP-832 in turn was discarded in favor of yet another KIP, KIP-839:
Provide builders for KafkaProducer/KafkaConsumer and KafkaStreams
<https://cwiki.apache.org/confluence/x/YBqhD>

Ultimately it seems like none of these KIPs were accepted or implemented so
the problem is still there. I'm also personally not sure why/how KIP-832
subsumes KIP-378 as it seems to address only the client supplier part of
the problem, whereas my understanding is that all statically
created/configured StreamsConfig objects would be affected
(eg DeserializationExceptionHandler)

Anyways, whatever the specifics, all of these KIPs were discarded at some
point. And IIUC the StreamsConfig overload is a necessary but sufficient
condition here -- that is, they needed the StreamsConfig overloads as part
of the original fix, but also had to make some further changes in order to
enable proper dependency injection in Spring like they wanted. So just
having a constructor that accepts StreamsConfig isn't doing anything
without the rest of the fix (ie KIP-378) and therefore it doesn't really
make sense to offer a StreamsConfig overload unless/until we have the full
set of changes.

So this definitely seems to answer the question for the
StreamsBuilder/Topology constructors, that is, no need for a StreamsConfig
version of these APIs. Which is enough to proceed with this KIP.

But it does kind of raise another question, which is, should we just
re-deprecate the StreamsConfig overloads of KafkaStreams? It seems like
when (or more likely, if) the dependency injection stuff is picked up and
finished then we can add these overloads back in as part of that KIP. Until
then, they're just increasing the surface area of the already bloated
KafkaStreams constructor.

Assuming we do want to re-deprecate these, would we need a KIP? There was
already a KIP to deprecate them in the first place of course (KIP-245
<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor>)
but what's interesting is that it seems we never did a KIP to *remove* the
deprecation. Seems like we were evaluating whether or not to remove these
KafkaStreams constructors during a major release and decided against doing
so due to the concerns raised in the discussion I linked to. And I guess we
decided that if we weren't going to remove them then we should just go
ahead and un-deprecate them...or something like that (#10484
<https://github.com/apache/kafka/pull/10484>). So...if there was no KIP to
un-deprecate them, can we re-deprecate them without a KIP?



On Wed, Apr 2, 2025 at 10:02 PM Sophie Blee-Goldman <so...@responsive.dev>
wrote:

> Sounds like we are in agreement and can move forward with this KIP.
>
> To sum, instead of having the new StreamsBuilder and Topology constructors
> take a StreamsConfig parameter, they should take a Map or Properties
> instead.
>
> Between the two of those I don't personally have much of a preference, and
> some clients (like KafkaConsumer) even seem to offer both a Map and a
> Properties version of the constructor. Imho this is a bit of an overkill
> and choosing one or the other should be fine for us. I'll note that the the
> KafkaStreams constructor uses Properties so perhaps that's the appropriate
> choice here (even if I personally like the plain Map option slightly better)
>
> Any final thoughts?
>
> On Thu, Mar 27, 2025 at 9:17 AM Bill Bejeck <bb...@gmail.com> wrote:
>
>> Hi All,
>>
>> I also agree with the having consistency with the other clients, so I'm
>> also a +1 for using Properties/Map.
>>
>> Thanks,
>> Bill
>>
>> On Thu, Mar 27, 2025 at 7:28 AM Bruno Cadonna <ca...@apache.org> wrote:
>>
>> > Hi,
>> >
>> > I proposed to use constructors with StreamsConfig because I wanted a
>> > public API that says to the users: "The config you pass to Streams will
>> > not be modified." That the first call in a constructor is a
>> > transformation from Properties/Map to StreamsConfig a user might or
>> > might not know.
>> >
>> > I looked a bit around and found KIP-245[1] that deprecated KafkaStreams
>> > constructors that take a StreamsConfig. The main motivation was that
>> > those constructors add boilerplate because you need to construct an
>> > additional object. Fair enough!
>> >
>> > Apparently, we also allow to modify the config in constructors[2].
>> >
>> > So the immutability property is not even possible without making larger
>> > changes.
>> >
>> > I agree that we should keep it consistent with the other clients and I
>> > also see the simplicity argument. So I am fine with just using
>> > Properties/Map.
>> >
>> > Would be interesting to understand what the problem with the dependency
>> > injection is. In the end a StreamsConfig is constructed from a Map.
>> >
>> >
>> > Best,
>> > Bruno
>> >
>> >
>> > [1]
>> >
>> >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor
>> > [2]
>> >
>> >
>> https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600
>> >
>> > On 26.03.25 22:33, Sophie Blee-Goldman wrote:
>> > > I think Lucas makes a good point. If we want to maintain consistency
>> with
>> > > the currents state and intended direction of the clients w.r.t config
>> > > classes, we should definitely (and perhaps only) offer the
>> Map/Properties
>> > > version of the APIs
>> > >
>> > > I even submitted a KIP a while back to open up the visibility of one
>> of
>> > the
>> > > ProducerConfig constructors, just  to bring it in line with the other
>> > > config classes (and to help shut off excessive logging), and it was
>> > pretty
>> > > much stopped dead by core/clients people who didn't like the current
>> > > openness of these config classes and wanted to move in the opposite
>> > > direction rather than make anything more visible (Even if just to
>> bring
>> > one
>> > > of them to parity with the other classes)
>> > >
>> > > I think this is a pretty good argument for at the very least offering
>> the
>> > > plain Map/Properties version, and perhaps only offering that. IIRC the
>> > only
>> > > reason we added the StreamsConfig  version of the KafkaStreams
>> > constructors
>> > > was to enable dependency injection for spring kafka users. I have to
>> > wonder
>> > > if there is a way to fix this in spring kafka to allow Map/Properties
>> > based
>> > > configuration, rather than forcing KafkaStreams to allow StreamsConfig
>> > > constructors
>> > >
>> > > So as of this time I'm still personally in favor of having only the
>> > > Map/Properties option, though that still leaves open the question of
>> > which
>> > > one (I notice that KAfkaConsumer, for example, offers both -- but this
>> > > seems unnecessary and I think either Map or Properties, but not both,
>> is
>> > > sufficient)
>> > >
>> > > All this said I don't want to block the KIP forever on this question,
>> so
>> > I
>> > > think we should try to wrap things up one way or the other. If we
>> can't
>> > > come to a consensus I'd advocate to just offer both a Map/Properties
>> and
>> > a
>> > > StreamsConfig version. That way if/when we ever have to deprecate one
>> of
>> > > them, it's less disruptive to everyone who hopefully was already using
>> > the
>> > > other option (imo it's more likely we'd want/need to deprecate the
>> > > StreamsConfig version and I also feel this will be the less popular
>> > option
>> > > since it seems only needed by those using spring)
>> > >
>> > > Thoughts?
>> > >
>> > > On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy
>> > > <lb...@confluent.io.invalid> wrote:
>> > >
>> > >> Hi,
>> > >>
>> > >> I agree with the KIP, it makes sense to reduce the number of ways the
>> > >> configs get passed. I agree with Sophie that it doesn't make sense to
>> > >> rewrite the topology compilation just to delay reading the configs.
>> > >>
>> > >> The question StreamsConfig vs. Map/Properties is less clear-cut to
>> me.
>> > >> The immutability argument seems weak - if a parameter of type
>> > >> Properties / Map is converted into a StreamsConfig copy right in the
>> > >> various constructors, we'd get the same immutability benefit.
>> > >>
>> > >> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take
>> > >> Map<String, Object> or Properties, and explicitly restrict the
>> > >> visibility of constructors with ConsumerConfig/ProducerConfig/...,
>> and
>> > >> KafkaStreams would decide to go a separate route, allowing only the
>> > >> complex AbstractConfig class instead of the simple data type. This
>> > >> seems pretty inconsistent for the interfaces of a single open source
>> > >> project.
>> > >>
>> > >> Cheers,
>> > >> Lucas
>> > >>
>> > >> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
>> > >> <so...@responsive.dev> wrote:
>> > >>>
>> > >>> I don't believe it's possible to only pass the configurations into
>> > >>> KafkaStreams, at least not without a huge refactoring of how and
>> when
>> > the
>> > >>> topology is compiled. I looked into this for the processor wrapper
>> > config
>> > >>> since obviously it's preferable to not require configs be passed in
>> to
>> > >> the
>> > >>> Topology/StreamsBuilder, and wasn't able to make it work. If it's
>> > >> possible
>> > >>> at all, it would have significantly complicated the already-complex
>> > >>> topology construction code. I don't think it's worth it.
>> > >>>
>> > >>> If we really wanted to only pass configs into one place, I'd say we
>> > >> should
>> > >>> just require them for the Topology and StreeamsBuilder constructor,
>> and
>> > >> not
>> > >>> pass configs into KafkaStreams at all. But that would mean a
>> > >>> deprecation/code change affecting every single user out there, which
>> > >> feels
>> > >>> like unnecessary turmoil.
>> > >>>
>> > >>> As for whether to require StreamsConfig vs Map/Properties, I'm fine
>> > with
>> > >>> requiring StreamsConfig in theory, but since KafkaStreams allows
>> users
>> > to
>> > >>> pass in a plain config map then some users might not instantiate
>> > >>> StreamsConfig at all, and this would force them to do so. Maybe
>> that's
>> > >> not
>> > >>> such a bad thing though...so yeah, guess I'm fine with just the
>> > >>> StreamsConfig variant :)
>> > >>>
>> > >>> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org>
>> > wrote:
>> > >>>
>> > >>>> Thanks for the KIP, this is great improvement.
>> > >>>>
>> > >>>> I'm +1 to all the comments and changes in the KIP thus far.
>> > >>>>
>> > >>>> I also like the proposal from Bruno regarding only passing
>> > >> configurations
>> > >>>> to KafkaStreams.
>> > >>>> Regarding your comment about internal configuration propagation,
>> would
>> > >>>> setting the visiblitly on the Topology constructor to package
>> private
>> > >> or
>> > >>>> protected solve the issue?
>> > >>>> I'm not sure I'm addressing your concern entirely, but if we can
>> > >> simplify
>> > >>>> the code paths for passing configuration, I think it's worth some
>> > >> tradeoffs
>> > >>>> for the gain in simplicity.
>> > >>>> As I'm writing this though I'm wondering if this is indeed possible
>> > >> when
>> > >>>> taking the PAPI into consideration.  At any rate, I think the KIP
>> can
>> > >>>> proceed as is, I just wanted to add my 2cents.
>> > >>>>
>> > >>>> Regards,
>> > >>>> Bill
>> > >>>>
>> > >>>>
>> > >>>> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
>> > >>>> sebastien.viale@michelin.com> wrote:
>> > >>>>
>> > >>>>> Hi,
>> > >>>>>
>> > >>>>> Thank you both for your remarks.
>> > >>>>>
>> > >>>>> Sophie:
>> > >>>>>
>> > >>>>> I have added all your proposals to the rejected alternatives.
>> > >>>>>
>> > >>>>> BC1:
>> > >>>>> I agree that we should not create multiple config classes, as
>> > >> avoiding
>> > >>>>> that is the main goal of the KIP.
>> > >>>>>
>> > >>>>> It would be nice to have a single constructor (KafkaStreams) that
>> > >> accepts
>> > >>>>> the configuration. However, unless I’m mistaken, this would
>> require
>> > >>>>> handling config propagation internally to other components like
>> > >> Topology.
>> > >>>>>
>> > >>>>> BC2:
>> > >>>>> StreamsConfig is indeed immutable, and like Bruno, I believe this
>> is
>> > >> the
>> > >>>>> safer approach.
>> > >>>>> Best,
>> > >>>>> Sébastien
>> > >>>>>
>> > >>>>>
>> > >>>>> De : Bruno Cadonna <ca...@apache.org>
>> > >>>>> Date : vendredi, 7 mars 2025 à 18:55
>> > >>>>> À : dev@kafka.apache.org <de...@kafka.apache.org>
>> > >>>>> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and
>> > >> API for
>> > >>>>> supplying configs needed by the topology
>> > >>>>> Warning External sender Do not click on any links or open any
>> > >> attachments
>> > >>>>> unless you trust the sender and know the content is safe.
>> > >>>>>
>> > >>>>> Hi Sébastien,
>> > >>>>>
>> > >>>>> Thanks for the KIP!
>> > >>>>>
>> > >>>>> Cleaning up the configs classes is really a good idea!
>> > >>>>>
>> > >>>>> BC1:
>> > >>>>> I am in favor of having only StreamsConfig as a config class to
>> avoid
>> > >>>>> proliferation of config classes. However, I find it a bit ugly
>> that a
>> > >>>>> Streams user is required to pass the config first to the topology
>> and
>> > >>>>> then to KafkaStreams. Since we are talking about changing
>> > >> constructors,
>> > >>>>> I am wondering if it were possible to pass the configs only to
>> > >>>>> KafkaStreams.
>> > >>>>>
>> > >>>>> BC2:
>> > >>>>> While Sophie advocates to only pass a Map/Properties object into
>> the
>> > >>>>> constructors, I am advocating to only passing StreamsConfig
>> objects
>> > >> to
>> > >>>>> the constructors. The reason is that Map/Properties are mutable
>> and
>> > >>>>> StreamsConfig is immutable. IMO, it would be safer to pass
>> immutable
>> > >>>>> configs to the constructors. But maybe there is a good reason to
>> pass
>> > >>>>> mutable configs that I do not know. Maybe Sophie can clarify.
>> > >>>>>
>> > >>>>> +1 to adding all considerations discussed in the thread to the
>> > >> rejected
>> > >>>>> alternatives.
>> > >>>>>
>> > >>>>>
>> > >>>>> Best,
>> > >>>>> Bruno
>> > >>>>> This email was screened for spam and malicious content but
>> exercise
>> > >>>>> caution anyway.
>> > >>>>>
>> > >>>>>
>> > >>>>>
>> > >>>>>
>> > >>>>>
>> > >>>>> On 06.03.25 02:43, Sophie Blee-Goldman wrote:
>> > >>>>>> Thanks for the KIP! The proposal here sounds like the right
>> > >> direction
>> > >>>> to
>> > >>>>> go
>> > >>>>>> in for me, but I'll be interested to hear what others think.
>> > >>>>>>
>> > >>>>>> I'm wondering if we should offer additional StreamsBuilder &
>> > >> Topology
>> > >>>>>> constructors that accept a plain config map (or Properties)
>> > >> alongside
>> > >>>> the
>> > >>>>>> StreamsConfig constructors. I'd advocate for providing only the
>> > >>>>>> Map/Properties variety except that I'm pretty sure the
>> > >> StreamsConfig
>> > >>>>>> options are needed for dependency injection and were requested by
>> > >> users
>> > >>>>> of
>> > >>>>>> Spring
>> > >>>>>>
>> > >>>>>> Also, just to list out some alternatives, we could go in a
>> > >> different
>> > >>>>>> direction completely by making TopologyConfig a true config in
>> its
>> > >> own
>> > >>>>>> right, and moving all the topology-specific configs from
>> > >> StreamsConfig
>> > >>>> to
>> > >>>>>> TopologyConfig (ie removing them from StreamsConfig completely)
>> > >>>>>>
>> > >>>>>> On the one hand, it might make more sense to keep configs
>> > >> separated and
>> > >>>>>> group them by where they need to be applied. On the other hand I
>> > >>>> wouldn't
>> > >>>>>> want to start proliferating config classes since I think this
>> makes
>> > >>>>>> discovery more difficult and increases the API surface area. So
>> > >>>>> personally,
>> > >>>>>> I think it's sufficient to just add the check and warning log
>> > >> described
>> > >>>>> in
>> > >>>>>> the KIP
>> > >>>>>>
>> > >>>>>> We could also deprecate the default constructor for
>> > >>>>> TopologyStreamsBuilder
>> > >>>>>> to force people to pass configs in. I'm honestly fine either way,
>> > >> as I
>> > >>>>>> don't really think it's much of a burden to ask users to pass
>> > >> configs
>> > >>>>> into
>> > >>>>>> one more place, but since the new check should help prevent users
>> > >> from
>> > >>>>>> accidentally forgetting to pass the topology-specific configs
>> into
>> > >> the
>> > >>>>>> Topology/StreamsBuilder, then deprecating the default
>> constructors
>> > >> seem
>> > >>>>>> like na unnecessary step.
>> > >>>>>>
>> > >>>>>> Sébastien -- My only other feedback for you on the KIP would be
>> to
>> > >> list
>> > >>>>>> these under the Rejected Alternatives section so we know what the
>> > >> range
>> > >>>>> of
>> > >>>>>> options are.
>> > >>>>>>
>> > >>>>>> Thanks!
>> > >>>>>> -Sophie
>> > >>>>>>
>> > >>>>>> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
>> > >>>>>> sebastien.viale@michelin.com> wrote:
>> > >>>>>>
>> > >>>>>>> Hi Everyone,
>> > >>>>>>>
>> > >>>>>>> I would like to start a discussion on KIP-1138: Clean up
>> > >>>> TopologyConfig
>> > >>>>>>> and API for supplying configs needed by the topology<
>> > >>>>>>>
>> > >>>>>
>> > >>>>
>> > >>
>> >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
>> > >>>>> <
>> > >>>>>
>> > >>>>
>> > >>
>> >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
>> > >>>>>>
>> > >>>>>>>>
>> > >>>>>>>
>> > >>>>>>> This proposal aims to simplify Kafka Streams configuration by
>> > >> unifying
>> > >>>>>>> APIs and ensuring topology-specific configs (e.g.,
>> > >>>>> topology.optimization,
>> > >>>>>>> processor.wrapper.class) are correctly applied to prevent silent
>> > >>>>>>> misconfigurations.
>> > >>>>>>>
>> > >>>>>>> Regards,
>> > >>>>>>>
>> > >>>>>>> Sébastien
>> > >>>>>>>
>> > >>>>>>>
>> > >>>>>>
>> > >>>>>
>> > >>>>
>> > >>
>> > >
>> >
>> >
>>
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Sophie Blee-Goldman <so...@responsive.dev>]

Sounds like we are in agreement and can move forward with this KIP.

To sum, instead of having the new StreamsBuilder and Topology constructors
take a StreamsConfig parameter, they should take a Map or Properties
instead.

Between the two of those I don't personally have much of a preference, and
some clients (like KafkaConsumer) even seem to offer both a Map and a
Properties version of the constructor. Imho this is a bit of an overkill
and choosing one or the other should be fine for us. I'll note that the the
KafkaStreams constructor uses Properties so perhaps that's the appropriate
choice here (even if I personally like the plain Map option slightly better)

Any final thoughts?

On Thu, Mar 27, 2025 at 9:17 AM Bill Bejeck <bb...@gmail.com> wrote:

> Hi All,
>
> I also agree with the having consistency with the other clients, so I'm
> also a +1 for using Properties/Map.
>
> Thanks,
> Bill
>
> On Thu, Mar 27, 2025 at 7:28 AM Bruno Cadonna <ca...@apache.org> wrote:
>
> > Hi,
> >
> > I proposed to use constructors with StreamsConfig because I wanted a
> > public API that says to the users: "The config you pass to Streams will
> > not be modified." That the first call in a constructor is a
> > transformation from Properties/Map to StreamsConfig a user might or
> > might not know.
> >
> > I looked a bit around and found KIP-245[1] that deprecated KafkaStreams
> > constructors that take a StreamsConfig. The main motivation was that
> > those constructors add boilerplate because you need to construct an
> > additional object. Fair enough!
> >
> > Apparently, we also allow to modify the config in constructors[2].
> >
> > So the immutability property is not even possible without making larger
> > changes.
> >
> > I agree that we should keep it consistent with the other clients and I
> > also see the simplicity argument. So I am fine with just using
> > Properties/Map.
> >
> > Would be interesting to understand what the problem with the dependency
> > injection is. In the end a StreamsConfig is constructed from a Map.
> >
> >
> > Best,
> > Bruno
> >
> >
> > [1]
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor
> > [2]
> >
> >
> https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600
> >
> > On 26.03.25 22:33, Sophie Blee-Goldman wrote:
> > > I think Lucas makes a good point. If we want to maintain consistency
> with
> > > the currents state and intended direction of the clients w.r.t config
> > > classes, we should definitely (and perhaps only) offer the
> Map/Properties
> > > version of the APIs
> > >
> > > I even submitted a KIP a while back to open up the visibility of one of
> > the
> > > ProducerConfig constructors, just  to bring it in line with the other
> > > config classes (and to help shut off excessive logging), and it was
> > pretty
> > > much stopped dead by core/clients people who didn't like the current
> > > openness of these config classes and wanted to move in the opposite
> > > direction rather than make anything more visible (Even if just to bring
> > one
> > > of them to parity with the other classes)
> > >
> > > I think this is a pretty good argument for at the very least offering
> the
> > > plain Map/Properties version, and perhaps only offering that. IIRC the
> > only
> > > reason we added the StreamsConfig  version of the KafkaStreams
> > constructors
> > > was to enable dependency injection for spring kafka users. I have to
> > wonder
> > > if there is a way to fix this in spring kafka to allow Map/Properties
> > based
> > > configuration, rather than forcing KafkaStreams to allow StreamsConfig
> > > constructors
> > >
> > > So as of this time I'm still personally in favor of having only the
> > > Map/Properties option, though that still leaves open the question of
> > which
> > > one (I notice that KAfkaConsumer, for example, offers both -- but this
> > > seems unnecessary and I think either Map or Properties, but not both,
> is
> > > sufficient)
> > >
> > > All this said I don't want to block the KIP forever on this question,
> so
> > I
> > > think we should try to wrap things up one way or the other. If we can't
> > > come to a consensus I'd advocate to just offer both a Map/Properties
> and
> > a
> > > StreamsConfig version. That way if/when we ever have to deprecate one
> of
> > > them, it's less disruptive to everyone who hopefully was already using
> > the
> > > other option (imo it's more likely we'd want/need to deprecate the
> > > StreamsConfig version and I also feel this will be the less popular
> > option
> > > since it seems only needed by those using spring)
> > >
> > > Thoughts?
> > >
> > > On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy
> > > <lb...@confluent.io.invalid> wrote:
> > >
> > >> Hi,
> > >>
> > >> I agree with the KIP, it makes sense to reduce the number of ways the
> > >> configs get passed. I agree with Sophie that it doesn't make sense to
> > >> rewrite the topology compilation just to delay reading the configs.
> > >>
> > >> The question StreamsConfig vs. Map/Properties is less clear-cut to me.
> > >> The immutability argument seems weak - if a parameter of type
> > >> Properties / Map is converted into a StreamsConfig copy right in the
> > >> various constructors, we'd get the same immutability benefit.
> > >>
> > >> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take
> > >> Map<String, Object> or Properties, and explicitly restrict the
> > >> visibility of constructors with ConsumerConfig/ProducerConfig/..., and
> > >> KafkaStreams would decide to go a separate route, allowing only the
> > >> complex AbstractConfig class instead of the simple data type. This
> > >> seems pretty inconsistent for the interfaces of a single open source
> > >> project.
> > >>
> > >> Cheers,
> > >> Lucas
> > >>
> > >> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
> > >> <so...@responsive.dev> wrote:
> > >>>
> > >>> I don't believe it's possible to only pass the configurations into
> > >>> KafkaStreams, at least not without a huge refactoring of how and when
> > the
> > >>> topology is compiled. I looked into this for the processor wrapper
> > config
> > >>> since obviously it's preferable to not require configs be passed in
> to
> > >> the
> > >>> Topology/StreamsBuilder, and wasn't able to make it work. If it's
> > >> possible
> > >>> at all, it would have significantly complicated the already-complex
> > >>> topology construction code. I don't think it's worth it.
> > >>>
> > >>> If we really wanted to only pass configs into one place, I'd say we
> > >> should
> > >>> just require them for the Topology and StreeamsBuilder constructor,
> and
> > >> not
> > >>> pass configs into KafkaStreams at all. But that would mean a
> > >>> deprecation/code change affecting every single user out there, which
> > >> feels
> > >>> like unnecessary turmoil.
> > >>>
> > >>> As for whether to require StreamsConfig vs Map/Properties, I'm fine
> > with
> > >>> requiring StreamsConfig in theory, but since KafkaStreams allows
> users
> > to
> > >>> pass in a plain config map then some users might not instantiate
> > >>> StreamsConfig at all, and this would force them to do so. Maybe
> that's
> > >> not
> > >>> such a bad thing though...so yeah, guess I'm fine with just the
> > >>> StreamsConfig variant :)
> > >>>
> > >>> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org>
> > wrote:
> > >>>
> > >>>> Thanks for the KIP, this is great improvement.
> > >>>>
> > >>>> I'm +1 to all the comments and changes in the KIP thus far.
> > >>>>
> > >>>> I also like the proposal from Bruno regarding only passing
> > >> configurations
> > >>>> to KafkaStreams.
> > >>>> Regarding your comment about internal configuration propagation,
> would
> > >>>> setting the visiblitly on the Topology constructor to package
> private
> > >> or
> > >>>> protected solve the issue?
> > >>>> I'm not sure I'm addressing your concern entirely, but if we can
> > >> simplify
> > >>>> the code paths for passing configuration, I think it's worth some
> > >> tradeoffs
> > >>>> for the gain in simplicity.
> > >>>> As I'm writing this though I'm wondering if this is indeed possible
> > >> when
> > >>>> taking the PAPI into consideration.  At any rate, I think the KIP
> can
> > >>>> proceed as is, I just wanted to add my 2cents.
> > >>>>
> > >>>> Regards,
> > >>>> Bill
> > >>>>
> > >>>>
> > >>>> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
> > >>>> sebastien.viale@michelin.com> wrote:
> > >>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> Thank you both for your remarks.
> > >>>>>
> > >>>>> Sophie:
> > >>>>>
> > >>>>> I have added all your proposals to the rejected alternatives.
> > >>>>>
> > >>>>> BC1:
> > >>>>> I agree that we should not create multiple config classes, as
> > >> avoiding
> > >>>>> that is the main goal of the KIP.
> > >>>>>
> > >>>>> It would be nice to have a single constructor (KafkaStreams) that
> > >> accepts
> > >>>>> the configuration. However, unless I’m mistaken, this would require
> > >>>>> handling config propagation internally to other components like
> > >> Topology.
> > >>>>>
> > >>>>> BC2:
> > >>>>> StreamsConfig is indeed immutable, and like Bruno, I believe this
> is
> > >> the
> > >>>>> safer approach.
> > >>>>> Best,
> > >>>>> Sébastien
> > >>>>>
> > >>>>>
> > >>>>> De : Bruno Cadonna <ca...@apache.org>
> > >>>>> Date : vendredi, 7 mars 2025 à 18:55
> > >>>>> À : dev@kafka.apache.org <de...@kafka.apache.org>
> > >>>>> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and
> > >> API for
> > >>>>> supplying configs needed by the topology
> > >>>>> Warning External sender Do not click on any links or open any
> > >> attachments
> > >>>>> unless you trust the sender and know the content is safe.
> > >>>>>
> > >>>>> Hi Sébastien,
> > >>>>>
> > >>>>> Thanks for the KIP!
> > >>>>>
> > >>>>> Cleaning up the configs classes is really a good idea!
> > >>>>>
> > >>>>> BC1:
> > >>>>> I am in favor of having only StreamsConfig as a config class to
> avoid
> > >>>>> proliferation of config classes. However, I find it a bit ugly
> that a
> > >>>>> Streams user is required to pass the config first to the topology
> and
> > >>>>> then to KafkaStreams. Since we are talking about changing
> > >> constructors,
> > >>>>> I am wondering if it were possible to pass the configs only to
> > >>>>> KafkaStreams.
> > >>>>>
> > >>>>> BC2:
> > >>>>> While Sophie advocates to only pass a Map/Properties object into
> the
> > >>>>> constructors, I am advocating to only passing StreamsConfig objects
> > >> to
> > >>>>> the constructors. The reason is that Map/Properties are mutable and
> > >>>>> StreamsConfig is immutable. IMO, it would be safer to pass
> immutable
> > >>>>> configs to the constructors. But maybe there is a good reason to
> pass
> > >>>>> mutable configs that I do not know. Maybe Sophie can clarify.
> > >>>>>
> > >>>>> +1 to adding all considerations discussed in the thread to the
> > >> rejected
> > >>>>> alternatives.
> > >>>>>
> > >>>>>
> > >>>>> Best,
> > >>>>> Bruno
> > >>>>> This email was screened for spam and malicious content but exercise
> > >>>>> caution anyway.
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> > >>>>>> Thanks for the KIP! The proposal here sounds like the right
> > >> direction
> > >>>> to
> > >>>>> go
> > >>>>>> in for me, but I'll be interested to hear what others think.
> > >>>>>>
> > >>>>>> I'm wondering if we should offer additional StreamsBuilder &
> > >> Topology
> > >>>>>> constructors that accept a plain config map (or Properties)
> > >> alongside
> > >>>> the
> > >>>>>> StreamsConfig constructors. I'd advocate for providing only the
> > >>>>>> Map/Properties variety except that I'm pretty sure the
> > >> StreamsConfig
> > >>>>>> options are needed for dependency injection and were requested by
> > >> users
> > >>>>> of
> > >>>>>> Spring
> > >>>>>>
> > >>>>>> Also, just to list out some alternatives, we could go in a
> > >> different
> > >>>>>> direction completely by making TopologyConfig a true config in its
> > >> own
> > >>>>>> right, and moving all the topology-specific configs from
> > >> StreamsConfig
> > >>>> to
> > >>>>>> TopologyConfig (ie removing them from StreamsConfig completely)
> > >>>>>>
> > >>>>>> On the one hand, it might make more sense to keep configs
> > >> separated and
> > >>>>>> group them by where they need to be applied. On the other hand I
> > >>>> wouldn't
> > >>>>>> want to start proliferating config classes since I think this
> makes
> > >>>>>> discovery more difficult and increases the API surface area. So
> > >>>>> personally,
> > >>>>>> I think it's sufficient to just add the check and warning log
> > >> described
> > >>>>> in
> > >>>>>> the KIP
> > >>>>>>
> > >>>>>> We could also deprecate the default constructor for
> > >>>>> TopologyStreamsBuilder
> > >>>>>> to force people to pass configs in. I'm honestly fine either way,
> > >> as I
> > >>>>>> don't really think it's much of a burden to ask users to pass
> > >> configs
> > >>>>> into
> > >>>>>> one more place, but since the new check should help prevent users
> > >> from
> > >>>>>> accidentally forgetting to pass the topology-specific configs into
> > >> the
> > >>>>>> Topology/StreamsBuilder, then deprecating the default constructors
> > >> seem
> > >>>>>> like na unnecessary step.
> > >>>>>>
> > >>>>>> Sébastien -- My only other feedback for you on the KIP would be to
> > >> list
> > >>>>>> these under the Rejected Alternatives section so we know what the
> > >> range
> > >>>>> of
> > >>>>>> options are.
> > >>>>>>
> > >>>>>> Thanks!
> > >>>>>> -Sophie
> > >>>>>>
> > >>>>>> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> > >>>>>> sebastien.viale@michelin.com> wrote:
> > >>>>>>
> > >>>>>>> Hi Everyone,
> > >>>>>>>
> > >>>>>>> I would like to start a discussion on KIP-1138: Clean up
> > >>>> TopologyConfig
> > >>>>>>> and API for supplying configs needed by the topology<
> > >>>>>>>
> > >>>>>
> > >>>>
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> > >>>>> <
> > >>>>>
> > >>>>
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> > >>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> This proposal aims to simplify Kafka Streams configuration by
> > >> unifying
> > >>>>>>> APIs and ensuring topology-specific configs (e.g.,
> > >>>>> topology.optimization,
> > >>>>>>> processor.wrapper.class) are correctly applied to prevent silent
> > >>>>>>> misconfigurations.
> > >>>>>>>
> > >>>>>>> Regards,
> > >>>>>>>
> > >>>>>>> Sébastien
> > >>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>
> > >
> >
> >
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Bill Bejeck <bb...@gmail.com>]

Hi All,

I also agree with the having consistency with the other clients, so I'm
also a +1 for using Properties/Map.

Thanks,
Bill

On Thu, Mar 27, 2025 at 7:28 AM Bruno Cadonna <ca...@apache.org> wrote:

> Hi,
>
> I proposed to use constructors with StreamsConfig because I wanted a
> public API that says to the users: "The config you pass to Streams will
> not be modified." That the first call in a constructor is a
> transformation from Properties/Map to StreamsConfig a user might or
> might not know.
>
> I looked a bit around and found KIP-245[1] that deprecated KafkaStreams
> constructors that take a StreamsConfig. The main motivation was that
> those constructors add boilerplate because you need to construct an
> additional object. Fair enough!
>
> Apparently, we also allow to modify the config in constructors[2].
>
> So the immutability property is not even possible without making larger
> changes.
>
> I agree that we should keep it consistent with the other clients and I
> also see the simplicity argument. So I am fine with just using
> Properties/Map.
>
> Would be interesting to understand what the problem with the dependency
> injection is. In the end a StreamsConfig is constructed from a Map.
>
>
> Best,
> Bruno
>
>
> [1]
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor
> [2]
>
> https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600
>
> On 26.03.25 22:33, Sophie Blee-Goldman wrote:
> > I think Lucas makes a good point. If we want to maintain consistency with
> > the currents state and intended direction of the clients w.r.t config
> > classes, we should definitely (and perhaps only) offer the Map/Properties
> > version of the APIs
> >
> > I even submitted a KIP a while back to open up the visibility of one of
> the
> > ProducerConfig constructors, just  to bring it in line with the other
> > config classes (and to help shut off excessive logging), and it was
> pretty
> > much stopped dead by core/clients people who didn't like the current
> > openness of these config classes and wanted to move in the opposite
> > direction rather than make anything more visible (Even if just to bring
> one
> > of them to parity with the other classes)
> >
> > I think this is a pretty good argument for at the very least offering the
> > plain Map/Properties version, and perhaps only offering that. IIRC the
> only
> > reason we added the StreamsConfig  version of the KafkaStreams
> constructors
> > was to enable dependency injection for spring kafka users. I have to
> wonder
> > if there is a way to fix this in spring kafka to allow Map/Properties
> based
> > configuration, rather than forcing KafkaStreams to allow StreamsConfig
> > constructors
> >
> > So as of this time I'm still personally in favor of having only the
> > Map/Properties option, though that still leaves open the question of
> which
> > one (I notice that KAfkaConsumer, for example, offers both -- but this
> > seems unnecessary and I think either Map or Properties, but not both, is
> > sufficient)
> >
> > All this said I don't want to block the KIP forever on this question, so
> I
> > think we should try to wrap things up one way or the other. If we can't
> > come to a consensus I'd advocate to just offer both a Map/Properties and
> a
> > StreamsConfig version. That way if/when we ever have to deprecate one of
> > them, it's less disruptive to everyone who hopefully was already using
> the
> > other option (imo it's more likely we'd want/need to deprecate the
> > StreamsConfig version and I also feel this will be the less popular
> option
> > since it seems only needed by those using spring)
> >
> > Thoughts?
> >
> > On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy
> > <lb...@confluent.io.invalid> wrote:
> >
> >> Hi,
> >>
> >> I agree with the KIP, it makes sense to reduce the number of ways the
> >> configs get passed. I agree with Sophie that it doesn't make sense to
> >> rewrite the topology compilation just to delay reading the configs.
> >>
> >> The question StreamsConfig vs. Map/Properties is less clear-cut to me.
> >> The immutability argument seems weak - if a parameter of type
> >> Properties / Map is converted into a StreamsConfig copy right in the
> >> various constructors, we'd get the same immutability benefit.
> >>
> >> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take
> >> Map<String, Object> or Properties, and explicitly restrict the
> >> visibility of constructors with ConsumerConfig/ProducerConfig/..., and
> >> KafkaStreams would decide to go a separate route, allowing only the
> >> complex AbstractConfig class instead of the simple data type. This
> >> seems pretty inconsistent for the interfaces of a single open source
> >> project.
> >>
> >> Cheers,
> >> Lucas
> >>
> >> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
> >> <so...@responsive.dev> wrote:
> >>>
> >>> I don't believe it's possible to only pass the configurations into
> >>> KafkaStreams, at least not without a huge refactoring of how and when
> the
> >>> topology is compiled. I looked into this for the processor wrapper
> config
> >>> since obviously it's preferable to not require configs be passed in to
> >> the
> >>> Topology/StreamsBuilder, and wasn't able to make it work. If it's
> >> possible
> >>> at all, it would have significantly complicated the already-complex
> >>> topology construction code. I don't think it's worth it.
> >>>
> >>> If we really wanted to only pass configs into one place, I'd say we
> >> should
> >>> just require them for the Topology and StreeamsBuilder constructor, and
> >> not
> >>> pass configs into KafkaStreams at all. But that would mean a
> >>> deprecation/code change affecting every single user out there, which
> >> feels
> >>> like unnecessary turmoil.
> >>>
> >>> As for whether to require StreamsConfig vs Map/Properties, I'm fine
> with
> >>> requiring StreamsConfig in theory, but since KafkaStreams allows users
> to
> >>> pass in a plain config map then some users might not instantiate
> >>> StreamsConfig at all, and this would force them to do so. Maybe that's
> >> not
> >>> such a bad thing though...so yeah, guess I'm fine with just the
> >>> StreamsConfig variant :)
> >>>
> >>> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org>
> wrote:
> >>>
> >>>> Thanks for the KIP, this is great improvement.
> >>>>
> >>>> I'm +1 to all the comments and changes in the KIP thus far.
> >>>>
> >>>> I also like the proposal from Bruno regarding only passing
> >> configurations
> >>>> to KafkaStreams.
> >>>> Regarding your comment about internal configuration propagation, would
> >>>> setting the visiblitly on the Topology constructor to package private
> >> or
> >>>> protected solve the issue?
> >>>> I'm not sure I'm addressing your concern entirely, but if we can
> >> simplify
> >>>> the code paths for passing configuration, I think it's worth some
> >> tradeoffs
> >>>> for the gain in simplicity.
> >>>> As I'm writing this though I'm wondering if this is indeed possible
> >> when
> >>>> taking the PAPI into consideration.  At any rate, I think the KIP can
> >>>> proceed as is, I just wanted to add my 2cents.
> >>>>
> >>>> Regards,
> >>>> Bill
> >>>>
> >>>>
> >>>> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
> >>>> sebastien.viale@michelin.com> wrote:
> >>>>
> >>>>> Hi,
> >>>>>
> >>>>> Thank you both for your remarks.
> >>>>>
> >>>>> Sophie:
> >>>>>
> >>>>> I have added all your proposals to the rejected alternatives.
> >>>>>
> >>>>> BC1:
> >>>>> I agree that we should not create multiple config classes, as
> >> avoiding
> >>>>> that is the main goal of the KIP.
> >>>>>
> >>>>> It would be nice to have a single constructor (KafkaStreams) that
> >> accepts
> >>>>> the configuration. However, unless I’m mistaken, this would require
> >>>>> handling config propagation internally to other components like
> >> Topology.
> >>>>>
> >>>>> BC2:
> >>>>> StreamsConfig is indeed immutable, and like Bruno, I believe this is
> >> the
> >>>>> safer approach.
> >>>>> Best,
> >>>>> Sébastien
> >>>>>
> >>>>>
> >>>>> De : Bruno Cadonna <ca...@apache.org>
> >>>>> Date : vendredi, 7 mars 2025 à 18:55
> >>>>> À : dev@kafka.apache.org <de...@kafka.apache.org>
> >>>>> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and
> >> API for
> >>>>> supplying configs needed by the topology
> >>>>> Warning External sender Do not click on any links or open any
> >> attachments
> >>>>> unless you trust the sender and know the content is safe.
> >>>>>
> >>>>> Hi Sébastien,
> >>>>>
> >>>>> Thanks for the KIP!
> >>>>>
> >>>>> Cleaning up the configs classes is really a good idea!
> >>>>>
> >>>>> BC1:
> >>>>> I am in favor of having only StreamsConfig as a config class to avoid
> >>>>> proliferation of config classes. However, I find it a bit ugly that a
> >>>>> Streams user is required to pass the config first to the topology and
> >>>>> then to KafkaStreams. Since we are talking about changing
> >> constructors,
> >>>>> I am wondering if it were possible to pass the configs only to
> >>>>> KafkaStreams.
> >>>>>
> >>>>> BC2:
> >>>>> While Sophie advocates to only pass a Map/Properties object into the
> >>>>> constructors, I am advocating to only passing StreamsConfig objects
> >> to
> >>>>> the constructors. The reason is that Map/Properties are mutable and
> >>>>> StreamsConfig is immutable. IMO, it would be safer to pass immutable
> >>>>> configs to the constructors. But maybe there is a good reason to pass
> >>>>> mutable configs that I do not know. Maybe Sophie can clarify.
> >>>>>
> >>>>> +1 to adding all considerations discussed in the thread to the
> >> rejected
> >>>>> alternatives.
> >>>>>
> >>>>>
> >>>>> Best,
> >>>>> Bruno
> >>>>> This email was screened for spam and malicious content but exercise
> >>>>> caution anyway.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> >>>>>> Thanks for the KIP! The proposal here sounds like the right
> >> direction
> >>>> to
> >>>>> go
> >>>>>> in for me, but I'll be interested to hear what others think.
> >>>>>>
> >>>>>> I'm wondering if we should offer additional StreamsBuilder &
> >> Topology
> >>>>>> constructors that accept a plain config map (or Properties)
> >> alongside
> >>>> the
> >>>>>> StreamsConfig constructors. I'd advocate for providing only the
> >>>>>> Map/Properties variety except that I'm pretty sure the
> >> StreamsConfig
> >>>>>> options are needed for dependency injection and were requested by
> >> users
> >>>>> of
> >>>>>> Spring
> >>>>>>
> >>>>>> Also, just to list out some alternatives, we could go in a
> >> different
> >>>>>> direction completely by making TopologyConfig a true config in its
> >> own
> >>>>>> right, and moving all the topology-specific configs from
> >> StreamsConfig
> >>>> to
> >>>>>> TopologyConfig (ie removing them from StreamsConfig completely)
> >>>>>>
> >>>>>> On the one hand, it might make more sense to keep configs
> >> separated and
> >>>>>> group them by where they need to be applied. On the other hand I
> >>>> wouldn't
> >>>>>> want to start proliferating config classes since I think this makes
> >>>>>> discovery more difficult and increases the API surface area. So
> >>>>> personally,
> >>>>>> I think it's sufficient to just add the check and warning log
> >> described
> >>>>> in
> >>>>>> the KIP
> >>>>>>
> >>>>>> We could also deprecate the default constructor for
> >>>>> TopologyStreamsBuilder
> >>>>>> to force people to pass configs in. I'm honestly fine either way,
> >> as I
> >>>>>> don't really think it's much of a burden to ask users to pass
> >> configs
> >>>>> into
> >>>>>> one more place, but since the new check should help prevent users
> >> from
> >>>>>> accidentally forgetting to pass the topology-specific configs into
> >> the
> >>>>>> Topology/StreamsBuilder, then deprecating the default constructors
> >> seem
> >>>>>> like na unnecessary step.
> >>>>>>
> >>>>>> Sébastien -- My only other feedback for you on the KIP would be to
> >> list
> >>>>>> these under the Rejected Alternatives section so we know what the
> >> range
> >>>>> of
> >>>>>> options are.
> >>>>>>
> >>>>>> Thanks!
> >>>>>> -Sophie
> >>>>>>
> >>>>>> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> >>>>>> sebastien.viale@michelin.com> wrote:
> >>>>>>
> >>>>>>> Hi Everyone,
> >>>>>>>
> >>>>>>> I would like to start a discussion on KIP-1138: Clean up
> >>>> TopologyConfig
> >>>>>>> and API for supplying configs needed by the topology<
> >>>>>>>
> >>>>>
> >>>>
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> >>>>> <
> >>>>>
> >>>>
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> >>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>> This proposal aims to simplify Kafka Streams configuration by
> >> unifying
> >>>>>>> APIs and ensuring topology-specific configs (e.g.,
> >>>>> topology.optimization,
> >>>>>>> processor.wrapper.class) are correctly applied to prevent silent
> >>>>>>> misconfigurations.
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>>
> >>>>>>> Sébastien
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
> >
>
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Bruno Cadonna <ca...@apache.org>]

Hi,

I proposed to use constructors with StreamsConfig because I wanted a 
public API that says to the users: "The config you pass to Streams will 
not be modified." That the first call in a constructor is a 
transformation from Properties/Map to StreamsConfig a user might or 
might not know.

I looked a bit around and found KIP-245[1] that deprecated KafkaStreams 
constructors that take a StreamsConfig. The main motivation was that 
those constructors add boilerplate because you need to construct an 
additional object. Fair enough!

Apparently, we also allow to modify the config in constructors[2].

So the immutability property is not even possible without making larger 
changes.

I agree that we should keep it consistent with the other clients and I 
also see the simplicity argument. So I am fine with just using 
Properties/Map.

Would be interesting to understand what the problem with the dependency 
injection is. In the end a StreamsConfig is constructed from a Map.


Best,
Bruno


[1] 
https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor
[2] 
https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600

On 26.03.25 22:33, Sophie Blee-Goldman wrote:
> I think Lucas makes a good point. If we want to maintain consistency with
> the currents state and intended direction of the clients w.r.t config
> classes, we should definitely (and perhaps only) offer the Map/Properties
> version of the APIs
> 
> I even submitted a KIP a while back to open up the visibility of one of the
> ProducerConfig constructors, just  to bring it in line with the other
> config classes (and to help shut off excessive logging), and it was pretty
> much stopped dead by core/clients people who didn't like the current
> openness of these config classes and wanted to move in the opposite
> direction rather than make anything more visible (Even if just to bring one
> of them to parity with the other classes)
> 
> I think this is a pretty good argument for at the very least offering the
> plain Map/Properties version, and perhaps only offering that. IIRC the only
> reason we added the StreamsConfig  version of the KafkaStreams constructors
> was to enable dependency injection for spring kafka users. I have to wonder
> if there is a way to fix this in spring kafka to allow Map/Properties based
> configuration, rather than forcing KafkaStreams to allow StreamsConfig
> constructors
> 
> So as of this time I'm still personally in favor of having only the
> Map/Properties option, though that still leaves open the question of which
> one (I notice that KAfkaConsumer, for example, offers both -- but this
> seems unnecessary and I think either Map or Properties, but not both, is
> sufficient)
> 
> All this said I don't want to block the KIP forever on this question, so I
> think we should try to wrap things up one way or the other. If we can't
> come to a consensus I'd advocate to just offer both a Map/Properties and a
> StreamsConfig version. That way if/when we ever have to deprecate one of
> them, it's less disruptive to everyone who hopefully was already using the
> other option (imo it's more likely we'd want/need to deprecate the
> StreamsConfig version and I also feel this will be the less popular option
> since it seems only needed by those using spring)
> 
> Thoughts?
> 
> On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy
> <lb...@confluent.io.invalid> wrote:
> 
>> Hi,
>>
>> I agree with the KIP, it makes sense to reduce the number of ways the
>> configs get passed. I agree with Sophie that it doesn't make sense to
>> rewrite the topology compilation just to delay reading the configs.
>>
>> The question StreamsConfig vs. Map/Properties is less clear-cut to me.
>> The immutability argument seems weak - if a parameter of type
>> Properties / Map is converted into a StreamsConfig copy right in the
>> various constructors, we'd get the same immutability benefit.
>>
>> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take
>> Map<String, Object> or Properties, and explicitly restrict the
>> visibility of constructors with ConsumerConfig/ProducerConfig/..., and
>> KafkaStreams would decide to go a separate route, allowing only the
>> complex AbstractConfig class instead of the simple data type. This
>> seems pretty inconsistent for the interfaces of a single open source
>> project.
>>
>> Cheers,
>> Lucas
>>
>> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
>> <so...@responsive.dev> wrote:
>>>
>>> I don't believe it's possible to only pass the configurations into
>>> KafkaStreams, at least not without a huge refactoring of how and when the
>>> topology is compiled. I looked into this for the processor wrapper config
>>> since obviously it's preferable to not require configs be passed in to
>> the
>>> Topology/StreamsBuilder, and wasn't able to make it work. If it's
>> possible
>>> at all, it would have significantly complicated the already-complex
>>> topology construction code. I don't think it's worth it.
>>>
>>> If we really wanted to only pass configs into one place, I'd say we
>> should
>>> just require them for the Topology and StreeamsBuilder constructor, and
>> not
>>> pass configs into KafkaStreams at all. But that would mean a
>>> deprecation/code change affecting every single user out there, which
>> feels
>>> like unnecessary turmoil.
>>>
>>> As for whether to require StreamsConfig vs Map/Properties, I'm fine with
>>> requiring StreamsConfig in theory, but since KafkaStreams allows users to
>>> pass in a plain config map then some users might not instantiate
>>> StreamsConfig at all, and this would force them to do so. Maybe that's
>> not
>>> such a bad thing though...so yeah, guess I'm fine with just the
>>> StreamsConfig variant :)
>>>
>>> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org> wrote:
>>>
>>>> Thanks for the KIP, this is great improvement.
>>>>
>>>> I'm +1 to all the comments and changes in the KIP thus far.
>>>>
>>>> I also like the proposal from Bruno regarding only passing
>> configurations
>>>> to KafkaStreams.
>>>> Regarding your comment about internal configuration propagation, would
>>>> setting the visiblitly on the Topology constructor to package private
>> or
>>>> protected solve the issue?
>>>> I'm not sure I'm addressing your concern entirely, but if we can
>> simplify
>>>> the code paths for passing configuration, I think it's worth some
>> tradeoffs
>>>> for the gain in simplicity.
>>>> As I'm writing this though I'm wondering if this is indeed possible
>> when
>>>> taking the PAPI into consideration.  At any rate, I think the KIP can
>>>> proceed as is, I just wanted to add my 2cents.
>>>>
>>>> Regards,
>>>> Bill
>>>>
>>>>
>>>> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
>>>> sebastien.viale@michelin.com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> Thank you both for your remarks.
>>>>>
>>>>> Sophie:
>>>>>
>>>>> I have added all your proposals to the rejected alternatives.
>>>>>
>>>>> BC1:
>>>>> I agree that we should not create multiple config classes, as
>> avoiding
>>>>> that is the main goal of the KIP.
>>>>>
>>>>> It would be nice to have a single constructor (KafkaStreams) that
>> accepts
>>>>> the configuration. However, unless I’m mistaken, this would require
>>>>> handling config propagation internally to other components like
>> Topology.
>>>>>
>>>>> BC2:
>>>>> StreamsConfig is indeed immutable, and like Bruno, I believe this is
>> the
>>>>> safer approach.
>>>>> Best,
>>>>> Sébastien
>>>>>
>>>>>
>>>>> De : Bruno Cadonna <ca...@apache.org>
>>>>> Date : vendredi, 7 mars 2025 à 18:55
>>>>> À : dev@kafka.apache.org <de...@kafka.apache.org>
>>>>> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and
>> API for
>>>>> supplying configs needed by the topology
>>>>> Warning External sender Do not click on any links or open any
>> attachments
>>>>> unless you trust the sender and know the content is safe.
>>>>>
>>>>> Hi Sébastien,
>>>>>
>>>>> Thanks for the KIP!
>>>>>
>>>>> Cleaning up the configs classes is really a good idea!
>>>>>
>>>>> BC1:
>>>>> I am in favor of having only StreamsConfig as a config class to avoid
>>>>> proliferation of config classes. However, I find it a bit ugly that a
>>>>> Streams user is required to pass the config first to the topology and
>>>>> then to KafkaStreams. Since we are talking about changing
>> constructors,
>>>>> I am wondering if it were possible to pass the configs only to
>>>>> KafkaStreams.
>>>>>
>>>>> BC2:
>>>>> While Sophie advocates to only pass a Map/Properties object into the
>>>>> constructors, I am advocating to only passing StreamsConfig objects
>> to
>>>>> the constructors. The reason is that Map/Properties are mutable and
>>>>> StreamsConfig is immutable. IMO, it would be safer to pass immutable
>>>>> configs to the constructors. But maybe there is a good reason to pass
>>>>> mutable configs that I do not know. Maybe Sophie can clarify.
>>>>>
>>>>> +1 to adding all considerations discussed in the thread to the
>> rejected
>>>>> alternatives.
>>>>>
>>>>>
>>>>> Best,
>>>>> Bruno
>>>>> This email was screened for spam and malicious content but exercise
>>>>> caution anyway.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 06.03.25 02:43, Sophie Blee-Goldman wrote:
>>>>>> Thanks for the KIP! The proposal here sounds like the right
>> direction
>>>> to
>>>>> go
>>>>>> in for me, but I'll be interested to hear what others think.
>>>>>>
>>>>>> I'm wondering if we should offer additional StreamsBuilder &
>> Topology
>>>>>> constructors that accept a plain config map (or Properties)
>> alongside
>>>> the
>>>>>> StreamsConfig constructors. I'd advocate for providing only the
>>>>>> Map/Properties variety except that I'm pretty sure the
>> StreamsConfig
>>>>>> options are needed for dependency injection and were requested by
>> users
>>>>> of
>>>>>> Spring
>>>>>>
>>>>>> Also, just to list out some alternatives, we could go in a
>> different
>>>>>> direction completely by making TopologyConfig a true config in its
>> own
>>>>>> right, and moving all the topology-specific configs from
>> StreamsConfig
>>>> to
>>>>>> TopologyConfig (ie removing them from StreamsConfig completely)
>>>>>>
>>>>>> On the one hand, it might make more sense to keep configs
>> separated and
>>>>>> group them by where they need to be applied. On the other hand I
>>>> wouldn't
>>>>>> want to start proliferating config classes since I think this makes
>>>>>> discovery more difficult and increases the API surface area. So
>>>>> personally,
>>>>>> I think it's sufficient to just add the check and warning log
>> described
>>>>> in
>>>>>> the KIP
>>>>>>
>>>>>> We could also deprecate the default constructor for
>>>>> TopologyStreamsBuilder
>>>>>> to force people to pass configs in. I'm honestly fine either way,
>> as I
>>>>>> don't really think it's much of a burden to ask users to pass
>> configs
>>>>> into
>>>>>> one more place, but since the new check should help prevent users
>> from
>>>>>> accidentally forgetting to pass the topology-specific configs into
>> the
>>>>>> Topology/StreamsBuilder, then deprecating the default constructors
>> seem
>>>>>> like na unnecessary step.
>>>>>>
>>>>>> Sébastien -- My only other feedback for you on the KIP would be to
>> list
>>>>>> these under the Rejected Alternatives section so we know what the
>> range
>>>>> of
>>>>>> options are.
>>>>>>
>>>>>> Thanks!
>>>>>> -Sophie
>>>>>>
>>>>>> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
>>>>>> sebastien.viale@michelin.com> wrote:
>>>>>>
>>>>>>> Hi Everyone,
>>>>>>>
>>>>>>> I would like to start a discussion on KIP-1138: Clean up
>>>> TopologyConfig
>>>>>>> and API for supplying configs needed by the topology<
>>>>>>>
>>>>>
>>>>
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
>>>>> <
>>>>>
>>>>
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> This proposal aims to simplify Kafka Streams configuration by
>> unifying
>>>>>>> APIs and ensuring topology-specific configs (e.g.,
>>>>> topology.optimization,
>>>>>>> processor.wrapper.class) are correctly applied to prevent silent
>>>>>>> misconfigurations.
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> Sébastien
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
> 

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Sophie Blee-Goldman <so...@responsive.dev>]

I think Lucas makes a good point. If we want to maintain consistency with
the currents state and intended direction of the clients w.r.t config
classes, we should definitely (and perhaps only) offer the Map/Properties
version of the APIs

I even submitted a KIP a while back to open up the visibility of one of the
ProducerConfig constructors, just  to bring it in line with the other
config classes (and to help shut off excessive logging), and it was pretty
much stopped dead by core/clients people who didn't like the current
openness of these config classes and wanted to move in the opposite
direction rather than make anything more visible (Even if just to bring one
of them to parity with the other classes)

I think this is a pretty good argument for at the very least offering the
plain Map/Properties version, and perhaps only offering that. IIRC the only
reason we added the StreamsConfig  version of the KafkaStreams constructors
was to enable dependency injection for spring kafka users. I have to wonder
if there is a way to fix this in spring kafka to allow Map/Properties based
configuration, rather than forcing KafkaStreams to allow StreamsConfig
constructors

So as of this time I'm still personally in favor of having only the
Map/Properties option, though that still leaves open the question of which
one (I notice that KAfkaConsumer, for example, offers both -- but this
seems unnecessary and I think either Map or Properties, but not both, is
sufficient)

All this said I don't want to block the KIP forever on this question, so I
think we should try to wrap things up one way or the other. If we can't
come to a consensus I'd advocate to just offer both a Map/Properties and a
StreamsConfig version. That way if/when we ever have to deprecate one of
them, it's less disruptive to everyone who hopefully was already using the
other option (imo it's more likely we'd want/need to deprecate the
StreamsConfig version and I also feel this will be the less popular option
since it seems only needed by those using spring)

Thoughts?

On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy
<lb...@confluent.io.invalid> wrote:

> Hi,
>
> I agree with the KIP, it makes sense to reduce the number of ways the
> configs get passed. I agree with Sophie that it doesn't make sense to
> rewrite the topology compilation just to delay reading the configs.
>
> The question StreamsConfig vs. Map/Properties is less clear-cut to me.
> The immutability argument seems weak - if a parameter of type
> Properties / Map is converted into a StreamsConfig copy right in the
> various constructors, we'd get the same immutability benefit.
>
> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take
> Map<String, Object> or Properties, and explicitly restrict the
> visibility of constructors with ConsumerConfig/ProducerConfig/..., and
> KafkaStreams would decide to go a separate route, allowing only the
> complex AbstractConfig class instead of the simple data type. This
> seems pretty inconsistent for the interfaces of a single open source
> project.
>
> Cheers,
> Lucas
>
> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
> <so...@responsive.dev> wrote:
> >
> > I don't believe it's possible to only pass the configurations into
> > KafkaStreams, at least not without a huge refactoring of how and when the
> > topology is compiled. I looked into this for the processor wrapper config
> > since obviously it's preferable to not require configs be passed in to
> the
> > Topology/StreamsBuilder, and wasn't able to make it work. If it's
> possible
> > at all, it would have significantly complicated the already-complex
> > topology construction code. I don't think it's worth it.
> >
> > If we really wanted to only pass configs into one place, I'd say we
> should
> > just require them for the Topology and StreeamsBuilder constructor, and
> not
> > pass configs into KafkaStreams at all. But that would mean a
> > deprecation/code change affecting every single user out there, which
> feels
> > like unnecessary turmoil.
> >
> > As for whether to require StreamsConfig vs Map/Properties, I'm fine with
> > requiring StreamsConfig in theory, but since KafkaStreams allows users to
> > pass in a plain config map then some users might not instantiate
> > StreamsConfig at all, and this would force them to do so. Maybe that's
> not
> > such a bad thing though...so yeah, guess I'm fine with just the
> > StreamsConfig variant :)
> >
> > On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org> wrote:
> >
> > > Thanks for the KIP, this is great improvement.
> > >
> > > I'm +1 to all the comments and changes in the KIP thus far.
> > >
> > > I also like the proposal from Bruno regarding only passing
> configurations
> > > to KafkaStreams.
> > > Regarding your comment about internal configuration propagation, would
> > > setting the visiblitly on the Topology constructor to package private
> or
> > > protected solve the issue?
> > > I'm not sure I'm addressing your concern entirely, but if we can
> simplify
> > > the code paths for passing configuration, I think it's worth some
> tradeoffs
> > > for the gain in simplicity.
> > > As I'm writing this though I'm wondering if this is indeed possible
> when
> > > taking the PAPI into consideration.  At any rate, I think the KIP can
> > > proceed as is, I just wanted to add my 2cents.
> > >
> > > Regards,
> > > Bill
> > >
> > >
> > > On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
> > > sebastien.viale@michelin.com> wrote:
> > >
> > > > Hi,
> > > >
> > > > Thank you both for your remarks.
> > > >
> > > > Sophie:
> > > >
> > > > I have added all your proposals to the rejected alternatives.
> > > >
> > > > BC1:
> > > > I agree that we should not create multiple config classes, as
> avoiding
> > > > that is the main goal of the KIP.
> > > >
> > > > It would be nice to have a single constructor (KafkaStreams) that
> accepts
> > > > the configuration. However, unless I’m mistaken, this would require
> > > > handling config propagation internally to other components like
> Topology.
> > > >
> > > > BC2:
> > > > StreamsConfig is indeed immutable, and like Bruno, I believe this is
> the
> > > > safer approach.
> > > > Best,
> > > > Sébastien
> > > >
> > > >
> > > > De : Bruno Cadonna <ca...@apache.org>
> > > > Date : vendredi, 7 mars 2025 à 18:55
> > > > À : dev@kafka.apache.org <de...@kafka.apache.org>
> > > > Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and
> API for
> > > > supplying configs needed by the topology
> > > > Warning External sender Do not click on any links or open any
> attachments
> > > > unless you trust the sender and know the content is safe.
> > > >
> > > > Hi Sébastien,
> > > >
> > > > Thanks for the KIP!
> > > >
> > > > Cleaning up the configs classes is really a good idea!
> > > >
> > > > BC1:
> > > > I am in favor of having only StreamsConfig as a config class to avoid
> > > > proliferation of config classes. However, I find it a bit ugly that a
> > > > Streams user is required to pass the config first to the topology and
> > > > then to KafkaStreams. Since we are talking about changing
> constructors,
> > > > I am wondering if it were possible to pass the configs only to
> > > > KafkaStreams.
> > > >
> > > > BC2:
> > > > While Sophie advocates to only pass a Map/Properties object into the
> > > > constructors, I am advocating to only passing StreamsConfig objects
> to
> > > > the constructors. The reason is that Map/Properties are mutable and
> > > > StreamsConfig is immutable. IMO, it would be safer to pass immutable
> > > > configs to the constructors. But maybe there is a good reason to pass
> > > > mutable configs that I do not know. Maybe Sophie can clarify.
> > > >
> > > > +1 to adding all considerations discussed in the thread to the
> rejected
> > > > alternatives.
> > > >
> > > >
> > > > Best,
> > > > Bruno
> > > > This email was screened for spam and malicious content but exercise
> > > > caution anyway.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> > > > > Thanks for the KIP! The proposal here sounds like the right
> direction
> > > to
> > > > go
> > > > > in for me, but I'll be interested to hear what others think.
> > > > >
> > > > > I'm wondering if we should offer additional StreamsBuilder &
> Topology
> > > > > constructors that accept a plain config map (or Properties)
> alongside
> > > the
> > > > > StreamsConfig constructors. I'd advocate for providing only the
> > > > > Map/Properties variety except that I'm pretty sure the
> StreamsConfig
> > > > > options are needed for dependency injection and were requested by
> users
> > > > of
> > > > > Spring
> > > > >
> > > > > Also, just to list out some alternatives, we could go in a
> different
> > > > > direction completely by making TopologyConfig a true config in its
> own
> > > > > right, and moving all the topology-specific configs from
> StreamsConfig
> > > to
> > > > > TopologyConfig (ie removing them from StreamsConfig completely)
> > > > >
> > > > > On the one hand, it might make more sense to keep configs
> separated and
> > > > > group them by where they need to be applied. On the other hand I
> > > wouldn't
> > > > > want to start proliferating config classes since I think this makes
> > > > > discovery more difficult and increases the API surface area. So
> > > > personally,
> > > > > I think it's sufficient to just add the check and warning log
> described
> > > > in
> > > > > the KIP
> > > > >
> > > > > We could also deprecate the default constructor for
> > > > TopologyStreamsBuilder
> > > > > to force people to pass configs in. I'm honestly fine either way,
> as I
> > > > > don't really think it's much of a burden to ask users to pass
> configs
> > > > into
> > > > > one more place, but since the new check should help prevent users
> from
> > > > > accidentally forgetting to pass the topology-specific configs into
> the
> > > > > Topology/StreamsBuilder, then deprecating the default constructors
> seem
> > > > > like na unnecessary step.
> > > > >
> > > > > Sébastien -- My only other feedback for you on the KIP would be to
> list
> > > > > these under the Rejected Alternatives section so we know what the
> range
> > > > of
> > > > > options are.
> > > > >
> > > > > Thanks!
> > > > > -Sophie
> > > > >
> > > > > On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> > > > > sebastien.viale@michelin.com> wrote:
> > > > >
> > > > >> Hi Everyone,
> > > > >>
> > > > >> I would like to start a discussion on KIP-1138: Clean up
> > > TopologyConfig
> > > > >> and API for supplying configs needed by the topology<
> > > > >>
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> > > > <
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> > > > >
> > > > >>>
> > > > >>
> > > > >> This proposal aims to simplify Kafka Streams configuration by
> unifying
> > > > >> APIs and ensuring topology-specific configs (e.g.,
> > > > topology.optimization,
> > > > >> processor.wrapper.class) are correctly applied to prevent silent
> > > > >> misconfigurations.
> > > > >>
> > > > >> Regards,
> > > > >>
> > > > >> Sébastien
> > > > >>
> > > > >>
> > > > >
> > > >
> > >
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Lucas Brutschy <lb...@confluent.io.INVALID>]

Hi,

I agree with the KIP, it makes sense to reduce the number of ways the
configs get passed. I agree with Sophie that it doesn't make sense to
rewrite the topology compilation just to delay reading the configs.

The question StreamsConfig vs. Map/Properties is less clear-cut to me.
The immutability argument seems weak - if a parameter of type
Properties / Map is converted into a StreamsConfig copy right in the
various constructors, we'd get the same immutability benefit.

On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take
Map<String, Object> or Properties, and explicitly restrict the
visibility of constructors with ConsumerConfig/ProducerConfig/..., and
KafkaStreams would decide to go a separate route, allowing only the
complex AbstractConfig class instead of the simple data type. This
seems pretty inconsistent for the interfaces of a single open source
project.

Cheers,
Lucas

On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman
<so...@responsive.dev> wrote:
>
> I don't believe it's possible to only pass the configurations into
> KafkaStreams, at least not without a huge refactoring of how and when the
> topology is compiled. I looked into this for the processor wrapper config
> since obviously it's preferable to not require configs be passed in to the
> Topology/StreamsBuilder, and wasn't able to make it work. If it's possible
> at all, it would have significantly complicated the already-complex
> topology construction code. I don't think it's worth it.
>
> If we really wanted to only pass configs into one place, I'd say we should
> just require them for the Topology and StreeamsBuilder constructor, and not
> pass configs into KafkaStreams at all. But that would mean a
> deprecation/code change affecting every single user out there, which feels
> like unnecessary turmoil.
>
> As for whether to require StreamsConfig vs Map/Properties, I'm fine with
> requiring StreamsConfig in theory, but since KafkaStreams allows users to
> pass in a plain config map then some users might not instantiate
> StreamsConfig at all, and this would force them to do so. Maybe that's not
> such a bad thing though...so yeah, guess I'm fine with just the
> StreamsConfig variant :)
>
> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org> wrote:
>
> > Thanks for the KIP, this is great improvement.
> >
> > I'm +1 to all the comments and changes in the KIP thus far.
> >
> > I also like the proposal from Bruno regarding only passing configurations
> > to KafkaStreams.
> > Regarding your comment about internal configuration propagation, would
> > setting the visiblitly on the Topology constructor to package private or
> > protected solve the issue?
> > I'm not sure I'm addressing your concern entirely, but if we can simplify
> > the code paths for passing configuration, I think it's worth some tradeoffs
> > for the gain in simplicity.
> > As I'm writing this though I'm wondering if this is indeed possible when
> > taking the PAPI into consideration.  At any rate, I think the KIP can
> > proceed as is, I just wanted to add my 2cents.
> >
> > Regards,
> > Bill
> >
> >
> > On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
> > sebastien.viale@michelin.com> wrote:
> >
> > > Hi,
> > >
> > > Thank you both for your remarks.
> > >
> > > Sophie:
> > >
> > > I have added all your proposals to the rejected alternatives.
> > >
> > > BC1:
> > > I agree that we should not create multiple config classes, as avoiding
> > > that is the main goal of the KIP.
> > >
> > > It would be nice to have a single constructor (KafkaStreams) that accepts
> > > the configuration. However, unless I’m mistaken, this would require
> > > handling config propagation internally to other components like Topology.
> > >
> > > BC2:
> > > StreamsConfig is indeed immutable, and like Bruno, I believe this is the
> > > safer approach.
> > > Best,
> > > Sébastien
> > >
> > >
> > > De : Bruno Cadonna <ca...@apache.org>
> > > Date : vendredi, 7 mars 2025 à 18:55
> > > À : dev@kafka.apache.org <de...@kafka.apache.org>
> > > Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for
> > > supplying configs needed by the topology
> > > Warning External sender Do not click on any links or open any attachments
> > > unless you trust the sender and know the content is safe.
> > >
> > > Hi Sébastien,
> > >
> > > Thanks for the KIP!
> > >
> > > Cleaning up the configs classes is really a good idea!
> > >
> > > BC1:
> > > I am in favor of having only StreamsConfig as a config class to avoid
> > > proliferation of config classes. However, I find it a bit ugly that a
> > > Streams user is required to pass the config first to the topology and
> > > then to KafkaStreams. Since we are talking about changing constructors,
> > > I am wondering if it were possible to pass the configs only to
> > > KafkaStreams.
> > >
> > > BC2:
> > > While Sophie advocates to only pass a Map/Properties object into the
> > > constructors, I am advocating to only passing StreamsConfig objects to
> > > the constructors. The reason is that Map/Properties are mutable and
> > > StreamsConfig is immutable. IMO, it would be safer to pass immutable
> > > configs to the constructors. But maybe there is a good reason to pass
> > > mutable configs that I do not know. Maybe Sophie can clarify.
> > >
> > > +1 to adding all considerations discussed in the thread to the rejected
> > > alternatives.
> > >
> > >
> > > Best,
> > > Bruno
> > > This email was screened for spam and malicious content but exercise
> > > caution anyway.
> > >
> > >
> > >
> > >
> > >
> > > On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> > > > Thanks for the KIP! The proposal here sounds like the right direction
> > to
> > > go
> > > > in for me, but I'll be interested to hear what others think.
> > > >
> > > > I'm wondering if we should offer additional StreamsBuilder & Topology
> > > > constructors that accept a plain config map (or Properties) alongside
> > the
> > > > StreamsConfig constructors. I'd advocate for providing only the
> > > > Map/Properties variety except that I'm pretty sure the StreamsConfig
> > > > options are needed for dependency injection and were requested by users
> > > of
> > > > Spring
> > > >
> > > > Also, just to list out some alternatives, we could go in a different
> > > > direction completely by making TopologyConfig a true config in its own
> > > > right, and moving all the topology-specific configs from StreamsConfig
> > to
> > > > TopologyConfig (ie removing them from StreamsConfig completely)
> > > >
> > > > On the one hand, it might make more sense to keep configs separated and
> > > > group them by where they need to be applied. On the other hand I
> > wouldn't
> > > > want to start proliferating config classes since I think this makes
> > > > discovery more difficult and increases the API surface area. So
> > > personally,
> > > > I think it's sufficient to just add the check and warning log described
> > > in
> > > > the KIP
> > > >
> > > > We could also deprecate the default constructor for
> > > TopologyStreamsBuilder
> > > > to force people to pass configs in. I'm honestly fine either way, as I
> > > > don't really think it's much of a burden to ask users to pass configs
> > > into
> > > > one more place, but since the new check should help prevent users from
> > > > accidentally forgetting to pass the topology-specific configs into the
> > > > Topology/StreamsBuilder, then deprecating the default constructors seem
> > > > like na unnecessary step.
> > > >
> > > > Sébastien -- My only other feedback for you on the KIP would be to list
> > > > these under the Rejected Alternatives section so we know what the range
> > > of
> > > > options are.
> > > >
> > > > Thanks!
> > > > -Sophie
> > > >
> > > > On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> > > > sebastien.viale@michelin.com> wrote:
> > > >
> > > >> Hi Everyone,
> > > >>
> > > >> I would like to start a discussion on KIP-1138: Clean up
> > TopologyConfig
> > > >> and API for supplying configs needed by the topology<
> > > >>
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> > > <
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> > > >
> > > >>>
> > > >>
> > > >> This proposal aims to simplify Kafka Streams configuration by unifying
> > > >> APIs and ensuring topology-specific configs (e.g.,
> > > topology.optimization,
> > > >> processor.wrapper.class) are correctly applied to prevent silent
> > > >> misconfigurations.
> > > >>
> > > >> Regards,
> > > >>
> > > >> Sébastien
> > > >>
> > > >>
> > > >
> > >
> >

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Sophie Blee-Goldman <so...@responsive.dev>]

I don't believe it's possible to only pass the configurations into
KafkaStreams, at least not without a huge refactoring of how and when the
topology is compiled. I looked into this for the processor wrapper config
since obviously it's preferable to not require configs be passed in to the
Topology/StreamsBuilder, and wasn't able to make it work. If it's possible
at all, it would have significantly complicated the already-complex
topology construction code. I don't think it's worth it.

If we really wanted to only pass configs into one place, I'd say we should
just require them for the Topology and StreeamsBuilder constructor, and not
pass configs into KafkaStreams at all. But that would mean a
deprecation/code change affecting every single user out there, which feels
like unnecessary turmoil.

As for whether to require StreamsConfig vs Map/Properties, I'm fine with
requiring StreamsConfig in theory, but since KafkaStreams allows users to
pass in a plain config map then some users might not instantiate
StreamsConfig at all, and this would force them to do so. Maybe that's not
such a bad thing though...so yeah, guess I'm fine with just the
StreamsConfig variant :)

On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bb...@apache.org> wrote:

> Thanks for the KIP, this is great improvement.
>
> I'm +1 to all the comments and changes in the KIP thus far.
>
> I also like the proposal from Bruno regarding only passing configurations
> to KafkaStreams.
> Regarding your comment about internal configuration propagation, would
> setting the visiblitly on the Topology constructor to package private or
> protected solve the issue?
> I'm not sure I'm addressing your concern entirely, but if we can simplify
> the code paths for passing configuration, I think it's worth some tradeoffs
> for the gain in simplicity.
> As I'm writing this though I'm wondering if this is indeed possible when
> taking the PAPI into consideration.  At any rate, I think the KIP can
> proceed as is, I just wanted to add my 2cents.
>
> Regards,
> Bill
>
>
> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
> sebastien.viale@michelin.com> wrote:
>
> > Hi,
> >
> > Thank you both for your remarks.
> >
> > Sophie:
> >
> > I have added all your proposals to the rejected alternatives.
> >
> > BC1:
> > I agree that we should not create multiple config classes, as avoiding
> > that is the main goal of the KIP.
> >
> > It would be nice to have a single constructor (KafkaStreams) that accepts
> > the configuration. However, unless I’m mistaken, this would require
> > handling config propagation internally to other components like Topology.
> >
> > BC2:
> > StreamsConfig is indeed immutable, and like Bruno, I believe this is the
> > safer approach.
> > Best,
> > Sébastien
> >
> >
> > De : Bruno Cadonna <ca...@apache.org>
> > Date : vendredi, 7 mars 2025 à 18:55
> > À : dev@kafka.apache.org <de...@kafka.apache.org>
> > Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for
> > supplying configs needed by the topology
> > Warning External sender Do not click on any links or open any attachments
> > unless you trust the sender and know the content is safe.
> >
> > Hi Sébastien,
> >
> > Thanks for the KIP!
> >
> > Cleaning up the configs classes is really a good idea!
> >
> > BC1:
> > I am in favor of having only StreamsConfig as a config class to avoid
> > proliferation of config classes. However, I find it a bit ugly that a
> > Streams user is required to pass the config first to the topology and
> > then to KafkaStreams. Since we are talking about changing constructors,
> > I am wondering if it were possible to pass the configs only to
> > KafkaStreams.
> >
> > BC2:
> > While Sophie advocates to only pass a Map/Properties object into the
> > constructors, I am advocating to only passing StreamsConfig objects to
> > the constructors. The reason is that Map/Properties are mutable and
> > StreamsConfig is immutable. IMO, it would be safer to pass immutable
> > configs to the constructors. But maybe there is a good reason to pass
> > mutable configs that I do not know. Maybe Sophie can clarify.
> >
> > +1 to adding all considerations discussed in the thread to the rejected
> > alternatives.
> >
> >
> > Best,
> > Bruno
> > This email was screened for spam and malicious content but exercise
> > caution anyway.
> >
> >
> >
> >
> >
> > On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> > > Thanks for the KIP! The proposal here sounds like the right direction
> to
> > go
> > > in for me, but I'll be interested to hear what others think.
> > >
> > > I'm wondering if we should offer additional StreamsBuilder & Topology
> > > constructors that accept a plain config map (or Properties) alongside
> the
> > > StreamsConfig constructors. I'd advocate for providing only the
> > > Map/Properties variety except that I'm pretty sure the StreamsConfig
> > > options are needed for dependency injection and were requested by users
> > of
> > > Spring
> > >
> > > Also, just to list out some alternatives, we could go in a different
> > > direction completely by making TopologyConfig a true config in its own
> > > right, and moving all the topology-specific configs from StreamsConfig
> to
> > > TopologyConfig (ie removing them from StreamsConfig completely)
> > >
> > > On the one hand, it might make more sense to keep configs separated and
> > > group them by where they need to be applied. On the other hand I
> wouldn't
> > > want to start proliferating config classes since I think this makes
> > > discovery more difficult and increases the API surface area. So
> > personally,
> > > I think it's sufficient to just add the check and warning log described
> > in
> > > the KIP
> > >
> > > We could also deprecate the default constructor for
> > TopologyStreamsBuilder
> > > to force people to pass configs in. I'm honestly fine either way, as I
> > > don't really think it's much of a burden to ask users to pass configs
> > into
> > > one more place, but since the new check should help prevent users from
> > > accidentally forgetting to pass the topology-specific configs into the
> > > Topology/StreamsBuilder, then deprecating the default constructors seem
> > > like na unnecessary step.
> > >
> > > Sébastien -- My only other feedback for you on the KIP would be to list
> > > these under the Rejected Alternatives section so we know what the range
> > of
> > > options are.
> > >
> > > Thanks!
> > > -Sophie
> > >
> > > On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> > > sebastien.viale@michelin.com> wrote:
> > >
> > >> Hi Everyone,
> > >>
> > >> I would like to start a discussion on KIP-1138: Clean up
> TopologyConfig
> > >> and API for supplying configs needed by the topology<
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> > <
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> > >
> > >>>
> > >>
> > >> This proposal aims to simplify Kafka Streams configuration by unifying
> > >> APIs and ensuring topology-specific configs (e.g.,
> > topology.optimization,
> > >> processor.wrapper.class) are correctly applied to prevent silent
> > >> misconfigurations.
> > >>
> > >> Regards,
> > >>
> > >> Sébastien
> > >>
> > >>
> > >
> >
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Bill Bejeck <bb...@apache.org>]

Thanks for the KIP, this is great improvement.

I'm +1 to all the comments and changes in the KIP thus far.

I also like the proposal from Bruno regarding only passing configurations
to KafkaStreams.
Regarding your comment about internal configuration propagation, would
setting the visiblitly on the Topology constructor to package private or
protected solve the issue?
I'm not sure I'm addressing your concern entirely, but if we can simplify
the code paths for passing configuration, I think it's worth some tradeoffs
for the gain in simplicity.
As I'm writing this though I'm wondering if this is indeed possible when
taking the PAPI into consideration.  At any rate, I think the KIP can
proceed as is, I just wanted to add my 2cents.

Regards,
Bill


On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale <
sebastien.viale@michelin.com> wrote:

> Hi,
>
> Thank you both for your remarks.
>
> Sophie:
>
> I have added all your proposals to the rejected alternatives.
>
> BC1:
> I agree that we should not create multiple config classes, as avoiding
> that is the main goal of the KIP.
>
> It would be nice to have a single constructor (KafkaStreams) that accepts
> the configuration. However, unless I’m mistaken, this would require
> handling config propagation internally to other components like Topology.
>
> BC2:
> StreamsConfig is indeed immutable, and like Bruno, I believe this is the
> safer approach.
> Best,
> Sébastien
>
>
> De : Bruno Cadonna <ca...@apache.org>
> Date : vendredi, 7 mars 2025 à 18:55
> À : dev@kafka.apache.org <de...@kafka.apache.org>
> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for
> supplying configs needed by the topology
> Warning External sender Do not click on any links or open any attachments
> unless you trust the sender and know the content is safe.
>
> Hi Sébastien,
>
> Thanks for the KIP!
>
> Cleaning up the configs classes is really a good idea!
>
> BC1:
> I am in favor of having only StreamsConfig as a config class to avoid
> proliferation of config classes. However, I find it a bit ugly that a
> Streams user is required to pass the config first to the topology and
> then to KafkaStreams. Since we are talking about changing constructors,
> I am wondering if it were possible to pass the configs only to
> KafkaStreams.
>
> BC2:
> While Sophie advocates to only pass a Map/Properties object into the
> constructors, I am advocating to only passing StreamsConfig objects to
> the constructors. The reason is that Map/Properties are mutable and
> StreamsConfig is immutable. IMO, it would be safer to pass immutable
> configs to the constructors. But maybe there is a good reason to pass
> mutable configs that I do not know. Maybe Sophie can clarify.
>
> +1 to adding all considerations discussed in the thread to the rejected
> alternatives.
>
>
> Best,
> Bruno
> This email was screened for spam and malicious content but exercise
> caution anyway.
>
>
>
>
>
> On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> > Thanks for the KIP! The proposal here sounds like the right direction to
> go
> > in for me, but I'll be interested to hear what others think.
> >
> > I'm wondering if we should offer additional StreamsBuilder & Topology
> > constructors that accept a plain config map (or Properties) alongside the
> > StreamsConfig constructors. I'd advocate for providing only the
> > Map/Properties variety except that I'm pretty sure the StreamsConfig
> > options are needed for dependency injection and were requested by users
> of
> > Spring
> >
> > Also, just to list out some alternatives, we could go in a different
> > direction completely by making TopologyConfig a true config in its own
> > right, and moving all the topology-specific configs from StreamsConfig to
> > TopologyConfig (ie removing them from StreamsConfig completely)
> >
> > On the one hand, it might make more sense to keep configs separated and
> > group them by where they need to be applied. On the other hand I wouldn't
> > want to start proliferating config classes since I think this makes
> > discovery more difficult and increases the API surface area. So
> personally,
> > I think it's sufficient to just add the check and warning log described
> in
> > the KIP
> >
> > We could also deprecate the default constructor for
> TopologyStreamsBuilder
> > to force people to pass configs in. I'm honestly fine either way, as I
> > don't really think it's much of a burden to ask users to pass configs
> into
> > one more place, but since the new check should help prevent users from
> > accidentally forgetting to pass the topology-specific configs into the
> > Topology/StreamsBuilder, then deprecating the default constructors seem
> > like na unnecessary step.
> >
> > Sébastien -- My only other feedback for you on the KIP would be to list
> > these under the Rejected Alternatives section so we know what the range
> of
> > options are.
> >
> > Thanks!
> > -Sophie
> >
> > On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> > sebastien.viale@michelin.com> wrote:
> >
> >> Hi Everyone,
> >>
> >> I would like to start a discussion on KIP-1138: Clean up TopologyConfig
> >> and API for supplying configs needed by the topology<
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
> >
> >>>
> >>
> >> This proposal aims to simplify Kafka Streams configuration by unifying
> >> APIs and ensuring topology-specific configs (e.g.,
> topology.optimization,
> >> processor.wrapper.class) are correctly applied to prevent silent
> >> misconfigurations.
> >>
> >> Regards,
> >>
> >> Sébastien
> >>
> >>
> >
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Sebastien Viale <se...@michelin.com>]

Hi,

Thank you both for your remarks.

Sophie:

I have added all your proposals to the rejected alternatives.

BC1:
I agree that we should not create multiple config classes, as avoiding that is the main goal of the KIP.

It would be nice to have a single constructor (KafkaStreams) that accepts the configuration. However, unless I’m mistaken, this would require handling config propagation internally to other components like Topology.

BC2:
StreamsConfig is indeed immutable, and like Bruno, I believe this is the safer approach.
Best,
Sébastien


De : Bruno Cadonna <ca...@apache.org>
Date : vendredi, 7 mars 2025 à 18:55
À : dev@kafka.apache.org <de...@kafka.apache.org>
Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology
Warning External sender Do not click on any links or open any attachments unless you trust the sender and know the content is safe.

Hi Sébastien,

Thanks for the KIP!

Cleaning up the configs classes is really a good idea!

BC1:
I am in favor of having only StreamsConfig as a config class to avoid
proliferation of config classes. However, I find it a bit ugly that a
Streams user is required to pass the config first to the topology and
then to KafkaStreams. Since we are talking about changing constructors,
I am wondering if it were possible to pass the configs only to KafkaStreams.

BC2:
While Sophie advocates to only pass a Map/Properties object into the
constructors, I am advocating to only passing StreamsConfig objects to
the constructors. The reason is that Map/Properties are mutable and
StreamsConfig is immutable. IMO, it would be safer to pass immutable
configs to the constructors. But maybe there is a good reason to pass
mutable configs that I do not know. Maybe Sophie can clarify.

+1 to adding all considerations discussed in the thread to the rejected
alternatives.


Best,
Bruno
This email was screened for spam and malicious content but exercise caution anyway.





On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> Thanks for the KIP! The proposal here sounds like the right direction to go
> in for me, but I'll be interested to hear what others think.
>
> I'm wondering if we should offer additional StreamsBuilder & Topology
> constructors that accept a plain config map (or Properties) alongside the
> StreamsConfig constructors. I'd advocate for providing only the
> Map/Properties variety except that I'm pretty sure the StreamsConfig
> options are needed for dependency injection and were requested by users of
> Spring
>
> Also, just to list out some alternatives, we could go in a different
> direction completely by making TopologyConfig a true config in its own
> right, and moving all the topology-specific configs from StreamsConfig to
> TopologyConfig (ie removing them from StreamsConfig completely)
>
> On the one hand, it might make more sense to keep configs separated and
> group them by where they need to be applied. On the other hand I wouldn't
> want to start proliferating config classes since I think this makes
> discovery more difficult and increases the API surface area. So personally,
> I think it's sufficient to just add the check and warning log described in
> the KIP
>
> We could also deprecate the default constructor for TopologyStreamsBuilder
> to force people to pass configs in. I'm honestly fine either way, as I
> don't really think it's much of a burden to ask users to pass configs into
> one more place, but since the new check should help prevent users from
> accidentally forgetting to pass the topology-specific configs into the
> Topology/StreamsBuilder, then deprecating the default constructors seem
> like na unnecessary step.
>
> Sébastien -- My only other feedback for you on the KIP would be to list
> these under the Rejected Alternatives section so we know what the range of
> options are.
>
> Thanks!
> -Sophie
>
> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> sebastien.viale@michelin.com> wrote:
>
>> Hi Everyone,
>>
>> I would like to start a discussion on KIP-1138: Clean up TopologyConfig
>> and API for supplying configs needed by the topology<
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>
>>>
>>
>> This proposal aims to simplify Kafka Streams configuration by unifying
>> APIs and ensuring topology-specific configs (e.g., topology.optimization,
>> processor.wrapper.class) are correctly applied to prevent silent
>> misconfigurations.
>>
>> Regards,
>>
>> Sébastien
>>
>>
>

Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying configs needed by the topology

[Bruno Cadonna <ca...@apache.org>]

Hi Sébastien,

Thanks for the KIP!

Cleaning up the configs classes is really a good idea!

BC1:
I am in favor of having only StreamsConfig as a config class to avoid 
proliferation of config classes. However, I find it a bit ugly that a 
Streams user is required to pass the config first to the topology and 
then to KafkaStreams. Since we are talking about changing constructors, 
I am wondering if it were possible to pass the configs only to KafkaStreams.

BC2:
While Sophie advocates to only pass a Map/Properties object into the 
constructors, I am advocating to only passing StreamsConfig objects to 
the constructors. The reason is that Map/Properties are mutable and 
StreamsConfig is immutable. IMO, it would be safer to pass immutable 
configs to the constructors. But maybe there is a good reason to pass 
mutable configs that I do not know. Maybe Sophie can clarify.

+1 to adding all considerations discussed in the thread to the rejected 
alternatives.


Best,
Bruno



On 06.03.25 02:43, Sophie Blee-Goldman wrote:
> Thanks for the KIP! The proposal here sounds like the right direction to go
> in for me, but I'll be interested to hear what others think.
> 
> I'm wondering if we should offer additional StreamsBuilder & Topology
> constructors that accept a plain config map (or Properties) alongside the
> StreamsConfig constructors. I'd advocate for providing only the
> Map/Properties variety except that I'm pretty sure the StreamsConfig
> options are needed for dependency injection and were requested by users of
> Spring
> 
> Also, just to list out some alternatives, we could go in a different
> direction completely by making TopologyConfig a true config in its own
> right, and moving all the topology-specific configs from StreamsConfig to
> TopologyConfig (ie removing them from StreamsConfig completely)
> 
> On the one hand, it might make more sense to keep configs separated and
> group them by where they need to be applied. On the other hand I wouldn't
> want to start proliferating config classes since I think this makes
> discovery more difficult and increases the API surface area. So personally,
> I think it's sufficient to just add the check and warning log described in
> the KIP
> 
> We could also deprecate the default constructor for TopologyStreamsBuilder
> to force people to pass configs in. I'm honestly fine either way, as I
> don't really think it's much of a burden to ask users to pass configs into
> one more place, but since the new check should help prevent users from
> accidentally forgetting to pass the topology-specific configs into the
> Topology/StreamsBuilder, then deprecating the default constructors seem
> like na unnecessary step.
> 
> Sébastien -- My only other feedback for you on the KIP would be to list
> these under the Rejected Alternatives section so we know what the range of
> options are.
> 
> Thanks!
> -Sophie
> 
> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale <
> sebastien.viale@michelin.com> wrote:
> 
>> Hi Everyone,
>>
>> I would like to start a discussion on KIP-1138: Clean up TopologyConfig
>> and API for supplying configs needed by the topology<
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology
>>>
>>
>> This proposal aims to simplify Kafka Streams configuration by unifying
>> APIs and ensuring topology-specific configs (e.g., topology.optimization,
>> processor.wrapper.class) are correctly applied to prevent silent
>> misconfigurations.
>>
>> Regards,
>>
>> Sébastien
>>
>>
>