Thanks Becket. I don't have a strong preference for *And* naming convention, I just thought it was a little weird to have two offset APIs in the same class which used a different convention. I'll go either way if others disagree. The rest sounds good to me.
-Jason On Tue, Sep 13, 2016 at 5:43 PM, Becket Qin <becket....@gmail.com> wrote: > 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 > > > > > > > > > >
