Thanks for the review Jason. I am fine with either name. I was thinking that our Scala class names are in the format of *And* pattern while the Java class is more concise. e.g TopicAndPartition vs TopicPartition. But it seems not a general convention. OffsetAndTimestamp sounds good to me.
I just updated the KIP wiki to add the explanation of handling duplicate partitions in the ListOffsetRequest wire protocol. Regarding the support for query multiple timestamps on the same partition. How about we start with the current proposal and if we do identify there is a strong need for multiple timestamp support, I am happy to add that later. At this point it seems more of an efficiency optimization. So I just want to avoid unnecessarily complicate the protocol and risk the potential abuse. Thanks, Jiangjie (Becket) Qin On Tue, Sep 13, 2016 at 3:08 PM, Jason Gustafson <ja...@confluent.io> wrote: > Hey Becket, > > I looked at the most recent version and it's looking good to me. One very > minor question on naming: do you think OffsetAndTimestamp would be a more > consistent name than TimestampOffset given some of our other naming (e.g. > OffsetAndMetadata)? I put the "Offset" first since the name > "offsetsForTimes" implies that the offset is the object being searched > while the timestamp is just satellite data. Also, it would be nice to > clarify the handling of duplicate topics as mentioned in the KIP call. > > Thanks, > Jason > > On Sat, Sep 10, 2016 at 4:37 PM, Becket Qin <becket....@gmail.com> wrote: > > > Hi Ismael, > > > > Got it. I agree that it is safer to use java.lang.Long instead of > primitive > > long. Returning a null sounds reasonable in our case where performance is > > not a major concern. I will make the change in the KIP wiki. > > > > Regarding the consistency, I am not sure how big impact it would be if we > > get rid all the primitive type fields in the messages (which is a > backwards > > incompatible change). If it is a problem, we may still have the > > inconsistent representation of the missing values. > > > > Jiangjie (Becket) Qin > > > > On Sat, Sep 10, 2016 at 1:01 AM, Ismael Juma <ism...@juma.me.uk> wrote: > > > > > Becket, comments inline. > > > > > > On Fri, Sep 9, 2016 at 6:55 PM, Becket Qin <becket....@gmail.com> > wrote: > > > > > > > Completely agree that we should have a consistent representation of > > > > something missing/unknown in the code. > > > > > > > > My understanding of the convention is that -1 means "not > > > > available"/"unknown". For example, when producer request failed, the > > > offset > > > > we used in the callback is -1. And Message.NoTimestamp is also -1. > > > > > > > > > ProducerRecord uses a nullable timestamp instead of Record.NO_TIMESTAMP > > to > > > indicate a missing value though. That was the example in my original > > > message. > > > > > > Using -1 > > > > instead of null has at least two benefits. > > > > 1) it works for primitive type as well as classes. > > > > > > > > > > This isn't a benefit outside of very specific use-cases (were the cost > of > > > boxing is a problem). The wrapper classes can be used, after all. The > > > downsides of using -1 is that the type system doesn't help you > (there's a > > > reason why they're called magic values). If you see a `java.lang.Long`, > > you > > > have a hint that `null` is a valid value. The other problem with a -1 > > for a > > > timestamp is that you may do nonsensical comparisons against it without > > > error. When using `null`, it will fail-fast with a NPE so that you can > > fix > > > it (even better would be to get compile-time errors, but let's leave > that > > > out of this discussion for now). > > > > > > > > > > 2) it is easy to send via wire protocols > > > > > > > > > > I think it's important to distinguish what we do in the wire protocols > > > (which may be more low-level) with what we do in user facing APIs > (where > > > usability and safety are very important). > > > > > > Ismael > > > > > >
