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
>>
>
