Hi Francois,
Modifying this KIP or starting a new one is your call.
I guess the idea of the builder might cause some more discussions. So if
you want to get a working version released as soon as possible, I would
opt to get the current KIP implemented and create a new one for the
builder. If time is not an issue I would modify the current KIP.
But as I said: It's your call.
Best,
Bruno
On 11.05.22 11:01, François Rosière wrote:
To be clear, there is no problem for me to update the current KIP with the
builder approach.
It's not a lot of work in terms of code.
So, up to you. Let me know and I will do the necessary to go in one or the
other direction...
Thanks again for the feedbacks.
Le mer. 11 mai 2022 à 10:52, François Rosière <francois.rosi...@gmail.com>
a écrit :
Hello,
Builder is clearly the way to go for future releases of Kafka.
If we align streams, we would have 3 builders
new ConsumerBuilder<String, MyPojo>()
.withKeyDeserializer(<KEY_DESERIALIZER>)
.withValueDeserializer(<VALUE_DESERIALIZER>)
.withInterceptors(<LIST_OF_INTERCEPTORS>)
.withMetricsReporter(<METRICS_REPORTER>)
.build(<MAP_OR_PROPERTIES_OR_CONFIG>);
new ProducerBuilder<String, MyPojo>()
.withKeySerializer(<KEY_SERIALIZER>)
.withValueSerializer(<VALUE_SERIALIZER>)
.withInterceptors(<LIST_OF_INTERCEPTORS>)
.withPartitioner(<PARTITIONER>)
.withMetricsReporter(<METRICS_REPORTER>)
.build(<MAP_OR_PROPERTIES_OR_CONFIG>);
new KafkaStreamsBuilder(<TOPOLOGY>)
.withProducerInterceptors(<LIST_OF_PRODUER_INTERCEPTORS>)
.withConsumerInterceptors(<LIST_OF_CONSUMER_INTERCEPTORS>)
.withTime(<TIME>)
.withKafkaClientSupplier(<KAFKA_CLIENT_SUPPLIER>)
.withMetricsReporter(<METRICS_REPORTER>)
.build(<MAP_OR_PROPERTIES_OR_CONFIG>);
The builder property would always override the configuration "instances".
There is maybe other methods to add to the builders...
The map, properties or config could be given to the constructor of the
builder instead of the build method...
At the end, we may only keep one single constructor in the Producer,
Consumer and KafkaStreams obects.
@Chris, @Bruno, thank you for your replies and proposals. Do you want I
create another KIP explaining the builder approach or do you prefer to do
it?
Kr,
F.
Le mer. 11 mai 2022 à 09:46, Bruno Cadonna <cado...@apache.org> a écrit :
Hi Francois and Chris,
I find the idea of the builder interesting.
I think we should go ahead with the current KIP as it is to allow
Francois to fix his issue soon. If one of you or both want to push
forward the builder idea, feel free to create a new KIP and discuss it
with the community.
Regarding Francois' questions:
3. If the existing constructors should be removed, they need to be
marked as deprecated and removed in one of the next major releases.
5. Yes, I think Streams should be aligned.
Both questions should be discussed in the context of a new KIP about the
builder idea.
Best,
Bruno
On 11.05.22 04:24, Chris Egerton wrote:
Hi Francois,
Thanks for your thoughts. I think it's worth noting that in regards to
item
2, it's possible to explicitly declare the type parameters for a builder
without capturing it in a variable; for example:
KafkaProducer<String, Integer> p = new Builder<String, Integer>(...)
.withKeySerializer(new StringSerializer())
.withValueSerializer(new IntegerSerializer())
.build();
That aside, given the three binding votes already cast on the vote
thread,
it's probably too late to be worth changing direction at this point.
Thanks
for entertaining the proposal, and congratulations on your KIP!
Cheers,
Chris
On Tue, May 10, 2022 at 5:33 PM François Rosière <
francois.rosi...@gmail.com>
wrote:
Hello Chris,
Thanks for the feedback. Builders is definitely the pattern to apply
when
an object needs to be built using different arguments/combinations.
Here are my thoughts about the proposal:
1. The builder should only expose meaningful methods for the users
such as
the interceptors, the serializer/deserializer, partitioner, etc. A
method
like the configured instances is internal and should not be exposed if
we
don't want to expose the config itself. Using this internal method is
the
only way to solve the issue if the config is exposed.
2. As the key and value types are not given, a variable will need to be
created for the builder before being used. Otherwise, there is no way
to
infer the type correctly. Breaks a bit the inline usage with DSL style.
3. What about existing constructors, they would need to stay to keep
compatibility with existing o could they be removed in benefit of the
builder?
4. Having an access to the config also gives a way to also fine tune
other
aspects such as the logging related to the config. Log unused, skip
some
properties, etc.
5. What about streams? Shouldn't it be aligned?
So, to summarise, the KIP was a best effort solution to support already
configured instances related to both the producer and the consumer.
The builder will work, it's just a matter of deciding the best
approach...
for me, both solutions are fine, I just need a way to inject already
configured dependencies into the producers and consumers.
If we conclude, I will drop a PR on Github.
Kr,
F.
Le mar. 10 mai 2022 à 15:01, Chris Egerton <fearthecel...@gmail.com> a
écrit :
Hi Francois,
Thanks for the KIP! I sympathize with the issue you're facing and with
John's reluctance to let perfect be the enemy of good, and if KIP
freeze
were tomorrow, I think this would be good enough. Given that we still
have
some time to work with, I'd like to propose an alternative approach
and
see
what your thoughts are.
There are a few issues with the current client APIs that are closely
related to the KIP:
1. Too many constructors (there are currently four each for
KafkaProducer
and KafkaConsumer, yet they all do basically the same thing)
2. Lack of type safety with interceptors (you have no way to enforce
at
compile time that your ProducerInterceptor<String, Integer> is used
with
a
Serializer<String> and Serializer<Integer>, for example)
3. Inflexibility and inconsistency with instantiation of pluggable
interfaces (you can bring your own (de)serializers, but everything
else
gets instantiated and configured for you at producer/consumer
construction
time)
The KIP as it exists now will only address item 3, and will exacerbate
item
1.
In addition, there are a few new issues introduced by the KIP as it
exists
now:
1. Tighter coupling between the ProducerConfig/ConsumerConfig classes
and
the KafkaProducer/KafkaConsumer classes. Any change we make in the
future
that breaks either of these config classes in unexpected ways (but
which
does not break the KafkaProducer/KafkaConsumer constructors that do
not
accept these classes as parameters) will now have a much higher
chance to
also break a user's entire producer/consumer application.
2. Complexity for users like yourself who would like to override
behavior
in a ProducerConfig/ConsumerConfig in order to inject pre-instantiated
dependencies. The example in the KIP overrides
AbstractConfig::getConfiguredInstances [1] in order to achieve this.
But
there are two other overloaded variants of getConfiguredInstances, and
two
AbstractConfig::getConfiguredInstance methods that also exist. We'd
either
need to establish a dependency graph between these methods (e.g., some
methods are guaranteed to invoke another overloaded variant) as part
of
the
public API for the AbstractConfig, or users would need to override
every
single one of these methods in order to ensure that their code won't
break
at runtime after bumping their Kafka version.
I think introducing the builder pattern for KafkaProducer and
KafkaConsumer
would alleviate all of these issues. As a rough draft of what the API
might
look like for KafkaProducer:
public class Builder<K, V> {
private final Map<String, Object> props;
private Serializer<K> keySerializer;
private Serializer<V> valueSerializer;
private List<ProducerInterceptor<K, V>> interceptors;
private Map<String, Object> configuredInstances;
private Map<String, List<Object>> configuredInstanceLists;
public Builder(Map<String, Object> props) {
this.props = props;
this.interceptors = new ArrayList<>();
this.configuredInstances = new HashMap<>();
this.configuredInstanceLists = new HashMap<>();
}
// Use this serializer, if non-null
// Will take precedence over any serializer class specified in
the
properties for this producer
public Builder withKeySerializer(Serializer<K> serializer) {
this.keySerializer = serializer;
return this;
}
public Builder withValueSerializer(Serializer<V> serializer) {
this.valueSerializer = serializer;
return this;
}
// Use these interceptors (has no effect if null)
// Each must already be configured
// Will be combined with any interceptor classes also specified
in
the
properties for this producer
public Builder withInterceptors(List<ProducerInterceptor<K, V>>
interceptors) {
if (interceptors != null) {
this.interceptors.addAll(interceptors);
}
return this;
}
// Use this plugin instance, if non-null
// Must already be configured
// Will take precedence over any plugin class specified for the
same
property in the properties for this producer (wording here needs work
but
you get the idea)
public Builder withConfiguredInstance(String property, Object
instance)
{
this.configuredInstances.put(property, instance);
return this;
}
// Use these plugin instances (has no effect if null)
// Each must already be configured
// Will be combined with any plugin classes also specified for
the
same
property in the properties for this consumer
public Builder withConfiguredInstances(String property,
List<Object>
instances) {
this.configuredInstanceLists.put(property, instances);
return this;
}
public KafkaProducer<K, V> build() { ... }
}
Thoughts?
[1] -
https://kafka.apache.org/31/javadoc/org/apache/kafka/common/config/AbstractConfig.html#getConfiguredInstances(java.lang.String,java.lang.Class,java.util.Map)
Cheers,
Chris
On Mon, May 9, 2022 at 4:55 PM Bruno Cadonna <cado...@apache.org>
wrote:
Hi Francois,
I think you can go ahead and call for votes.
Could you please also clean up a little bit the KIP since it has
still
parts that refer to its first version? For example, "Compatibility,
Deprecation, and Migration Plan" still mentions only two
constructors.
IMO you can also remove section "Public Interfaces" since it does not
contain much information.
Best,
Bruno
On 09.05.22 17:45, Bruno Cadonna wrote:
Hi Francois,
You can open a PR and people can review it, but it must not be
merged
until the KIP is approved.
Best,
Bruno
On 09.05.22 16:07, François Rosière wrote:
Can a PR be dropped on Github or do we still need some approval
first?
Le dim. 8 mai 2022 à 06:08, John Roesler <vvcep...@apache.org> a
écrit
:
Thanks, François!
Those changes look good to me.
Thanks,
-John
On Fri, May 6, 2022, at 13:51, François Rosière wrote:
The KIP has been updated to reflect the last discussion
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578#KIP832:Allowcreatingaproducer/consumerusingaproducer/consumerconfig-ProposedChanges
Le ven. 6 mai 2022 à 20:44, François Rosière
<francois.rosi...@gmail.com>
a
écrit :
Hello,
No problem to also add a constructor taking the StreamsConfig in
the
TopologyTestDriver.
Summary about the changes to apply:
- Create 2 new constructors in KafkaProducer
- Create a new constructor in KafkaConsumer and increase de
visibility
of an existing one
- Create a new constructor in TopologyTestDriver
Kr,
F.
Le ven. 6 mai 2022 à 16:57, John Roesler <vvcep...@apache.org>
a
écrit
:
Thanks for the KIP, François!
I'm generally in favor of your KIP, since you're
proposing to follow the existing pattern of the
constructors for both Producer and Consumer,
but with the config object instead of Properties
or Map configs. Also, because we already have
this pattern in Streams, and we are just
extending it to Producer and Consumer.
Following on the KIP-378 discussion, I do still think
this is somewhat of an abuse of the Config objects,
and it would be better to have a formal dependency
injection interface, but I also don't want to let perfect
be the enemy of good. Since it looks like this approach
works, and there is also some precedent for it already,
I'd be inclined to approve it.
Since KIP-378 didn't make it over the finish line, and it
seems like a small expansion to your proposal, do you
mind also adding the StreamsConfig to the
TopologyTestDriver constructors? That way, we can go
ahead and resolve both KIPs at once.
Thank you,
-John
On Fri, May 6, 2022, at 06:06, François Rosière wrote:
To stay consistent with existing code, we should simply add 2
constructors.
One with ser/deser and one without.
So that, users have the choice to use one or the other.
I updated the KIP accordingly.
Le ven. 6 mai 2022 à 12:55, François Rosière <
francois.rosi...@gmail.com> a
écrit :
On the other hand, the KafkaConsumer constructor with a
config +
serializer and deserializer already exists but is not public.
It would also complexify a bit the caller to not have the
serializer/deserializer exposed at constructor level.
Once the KIP would have been implemented, for streams,
instead
of
having a
custom config (already possible), I may simply define a
custom
KafkaClientSupplier reusing the custom configs of both the
producer
and the
consumer.
This supplier currently creates producers and consumers using
the
constructors with a map of config + serializer/deserializer.
So, it seems it's easier to have the constructor with 3
parameters.
But in
any case, it will work if the config can be accessed...
Le ven. 6 mai 2022 à 12:14, François Rosière <
francois.rosi...@gmail.com>
a écrit :
Hello,
We may create a constructor with a single parameter which is
the
config
but then, I would need to give the serializer/deserializer
by
also
overriding the config.
Like I would do for the interceptors.
So, no real opinion on that, both solutions are ok for me.
Maybe easier to take the approach of the single parameter.
Hope it respond to the question.
Kr,
F.
Le ven. 6 mai 2022 à 11:59, Bruno Cadonna <
cado...@apache.org>
a
écrit :
Hi Francois,
Thank you for updating the KIP!
Now the motivation of the KIP is much clearer.
I would still be interested in:
>> 2. Why do you only want to change/add the
constructors
that
take
the
>> properties objects and de/serializers and you do not
also
want to
>> add/change the constructors that take only the
properties?
Best,
Bruno
On 05.05.22 23:15, François Rosière wrote:
Hello Bruno,
The KIP as been updated. Feel free to give more feedbacks
and I
will
complete accordingly.
Kr,
F.
Le jeu. 5 mai 2022 à 22:22, Bruno Cadonna <
cado...@apache.org>
a
écrit :
Hi Francois,
Thanks for the KIP!
Here my first feedback:
1. Could you please extend the motivation section, so
that
it
is
clear
for a non-Spring dev why the change is needed? Usually, a
motivation
section benefits a lot from an actual example.
Extending the motivation section would also make the KIP
more
self-contained which is important IMO since this is kind
of
a
log
of
the
major changes to Kafka. Descriptions of major changes
should
not
completely depend on external links (which may become
dead
in
future).
Referencing external resources to point to more details
or
give
context
is useful, though.
2. Why do you only want to change/add the constructors
that
take
the
properties objects and de/serializers and you do not also
want
to
add/change the constructors that take only the
properties?
3. I found the following stalled KIP whose motivation is
really
similar
to yours:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-378%3A+Enable+Dependency+Injection+for+Kafka+Streams+handlers
That KIP is also the reason why Kafka Streams still has
the
constructors
with the StreamsConfig parameter. Maybe you want to
mention
this
KIP
in
yours or even incorporate the remaining topology test
driver
API
changes
in your KIP.
Some related links:
-
https://github.com/apache/kafka/pull/5344#issuecomment-413350338
- https://github.com/apache/kafka/pull/10484
- https://issues.apache.org/jira/browse/KAFKA-6386
Best,
Bruno
On 04.05.22 22:26, François Rosière wrote:
Hi all,
KIP-832 has been created to allow implementing Spring
managed
interceptors
for Producers and Consumers.
At the moment, interceptors are given as configuration
classes
to the
producer and consumer configurations. So, the idea here
would
be
to
create
2 new constructors to allow using a Producer and
Consumer
configuration
instead of properties or a key value map of
configurations
entries.
Interceptors could then be given as instances by
overriding a
config
method.
More details can be found in the Spring issue.
https://github.com/spring-projects/spring-kafka/issues/2244
Any feedback, proposal, vote for this KIP would be more
than
welcome.
Kind regards,
Francois R.
Le lun. 2 mai 2022 à 21:05, François Rosière <
francois.rosi...@gmail.com>
a
écrit :
Kip link:
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578
