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
<sop...@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 <bbej...@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.vi...@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 <cado...@apache.org>
Date : vendredi, 7 mars 2025 à 18:55
À : dev@kafka.apache.org <dev@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.vi...@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
