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