Agreed! I’ll update the KIP shortly
I’m self-taught, so I guess I still have a lot to learn (
If anything, I think the withForcedPartition method could be just removed, and
if users need to force partition, shouldn’t they just mandatorily use a custom
Partitioner? It would achieve the same purpose, but in a much “safer” way?
It just sounds like there would be two ways to achieve the same purpose here.
Nonetheless I understand it could be a breaking change
- Stephane
On 22/4/17, 4:05 am, "Matthias J. Sax" <matth...@confluent.io> wrote:
Ismael's comment is quite reasonable.
And I actually like the idea about `withForcedPartition` -- it helps
newcomers to understand the API better. Even if I don't agree with some
of you reasoning:
> I was always told that the key is solely the determining factor
Bad teacher?
> I find it incredibly unintuitive and dangerous to provide the users the
ability to force a partition
Disagree completely. This is a very useful feature for advanced use
cases. Btw: users can also specify a custom partitioner -- and this
custom partitioner could also do any computation to determine the
partitions (it also has access to key and value -- thus, it could also
use the value to compute the partition).
But I like `withForcedPartition` because it indicates that the
partitioner (default or custom) is not going to be used for this case.
Btw: if we plan to make `public ProducerRecord(String topic, Integer
partition, Long timestamp, K key, V value)` protected as some point, we
should deprecate it, too.
I also like the compromise you suggest
>> public ProducerRecordBuilder(String topic, K key, V value)
>>
>> coming with withPartition and withTimestamp?
The only thing, I still don't like is that we use a builder and are thus
forced to call .build() -- boilerplate.
Maybe we would just change ProducerRecord itself? Like:
new ProducerRecord(topic, key,
value).withTimestamp(ts).withForcedPartition(p);
WDYT?
-Matthias
On 4/20/17 4:57 PM, Stephane Maarek wrote:
> Matthias: I was definitely on board with you at first, but Ismael made
the comment that for:
>
> public ProducerRecord(String topic, K key, V value, Integer partition)
> public ProducerRecord(String topic, K key, V value, Long timestamp)
>
> Integer and Long are way too close in terms of meaning, and could provide
a strong misuse of the timestamp / partition field.
> Therefore I started with a builder pattern for explicit argument
declaration. Seems like a lot of boilerplate, but it makes things quite easy to
understand.
>
> I like your point about the necessity of the key, and that users should
set it to null explicitely.
>
> Damian: I like your idea of public ProducerRecordBuilder(String topic, V
value)
> Finally, I also chose the withForcedPartition because in my learning of
Kafka, I was always told that the key is solely the determining factor to know
how a messages makes it to a partition. I find it incredibly unintuitive and
dangerous to provide the users the ability to force a partition. If anything
they should be providing their own key -> partition mapping, but I’m really
against letting users force a partition within the producerRecord. What do you
think?
>
>
> What do you both think of the more opiniated:
>
> public ProducerRecordBuilder(String topic, K key, V value)
>
> coming with withPartition and withTimestamp?
>
>
>
> On 21/4/17, 2:24 am, "Matthias J. Sax" <matth...@confluent.io> wrote:
>
> Thanks for the KIP!
>
> While I agree, that the current API is not perfect, I am not sure if a
> builder pattern does make sense here, because it's not too many
parameters.
>
> IMHO, builder pattern makes sense if there are many optional
parameters.
> For a ProducerRecord, I think there are only 2 optional parameters:
> partition and timestamp.
>
> I don't think key should be optional, because uses should be "forced"
to
> think about the key argument as it effects the partitioning. Thus,
> providing an explicit `null` if there is no key seems reasonable to
me.
>
> Overall I think that providing 3 overloads would be sufficient:
>
> > public ProducerRecord(String topic, K key, V value, Integer
partition)
> > public ProducerRecord(String topic, K key, V value, Long timestamp)
> > public ProducerRecord(String topic, K key, V value, Integer
partition, Long timestamp)
>
>
> Just my 2 cents.
>
> -Matthias
>
>
> On 4/20/17 4:20 AM, Damian Guy wrote:
> > Hi Stephane,
> >
> > Thanks for the KIP. Overall it looks ok, though i think the
builder should
> > enforce the required parameters by supplying them via the
constructor, i.e,
> >
> > public ProducerRecordBuilder(String topic, V value)
> >
> > You can then remove the withValue and withTopic methods
> >
> > I also think withForcedPartition should just be withPartition
> >
> > Thanks,
> > Damian
> >
> > On Wed, 19 Apr 2017 at 23:34 Stephane Maarek
<steph...@simplemachines.com.au>
> > wrote:
> >
> >> Hi all,
> >>
> >> My first KIP, let me know your thoughts!
> >>
> >>
https://cwiki.apache.org/confluence/display/KAFKA/KIP+141+-+ProducerRecordBuilder+Interface
> >>
> >>
> >> Cheers,
> >> Stephane
> >>
> >
>
>
>
>
