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 >> >
signature.asc
Description: OpenPGP digital signature
