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