Hi Tom, Thanks for the updates. Overall, LGTM.
We probably want to add a new static method in `OffsetSpec` for `MaxTimestampSpec` similarly to what we did for the other specs. I thought a bit about Jason's suggestion. I do agree that it would be convenient to include the timestamp type in the response but it is not strictly necessary as you pointed out as one knows what it queries for. I don't have a strong opinion about this. Best, David On Wed, Apr 28, 2021 at 3:52 PM Thomas Scott <t...@confluent.io.invalid> wrote: > Hi David and Jason, > > Thank you for looking over the KIP. I've added extra text regarding > motivation and included the bump in API version required for the earlier > versions issue. > > Regarding returning the timestamp type, I was trying to keep the KIP > small and consistent with the existing behaviour (right now we return > timestamps for OffsetSpec.TimestampSpec and don't provide information on > the timestamp type). Also, I think the timestamp type is likely to be > something we can expect to be fairly constant (maybe i'm wrong on this) and > so fetching it on every call to listOffsets may be too frequent. > > Thanks > > Tom > > > > On Tue, Apr 27, 2021 at 11:24 PM Jason Gustafson > <ja...@confluent.io.invalid> > wrote: > > > Hi Tom, > > > > I had the same question as David. It sounds like this will require a bump > > to the `ListOffsets` API? Otherwise, looking at the code, the sentinel > > would be interpreted as a timestamp query and it looks like it would end > up > > returning the smallest timestamp ;). > > > > A second thought I had is whether it would be useful to include the > > timestamp type in the response. Otherwise it can be difficult to > interpret > > the timestamp value. > > > > Best, > > Jason > > > > On Tue, Apr 27, 2021 at 5:18 AM David Jacot <dja...@confluent.io.invalid > > > > wrote: > > > > > Hey Thomas, > > > > > > Thanks for the KIP. I have few comments/questions about it: > > > > > > 1. It would be great if we could better explain why we need this in > > > the motivation. At the moment, it only explains what will be added. > > > > > > 2. You plan to add the `MAX_TIMESTAMP` constant to tell the > > > broker to return the maximum timestamp. How do you plan to > > > handle the case where a new admin client would send this to > > > an old broker which does not support it? > > > > > > Best, > > > David > > > > > > On Tue, Apr 20, 2021 at 6:27 PM Thomas Scott <t...@confluent.io.invalid > > > > > wrote: > > > > > > > Hey all, > > > > > > > > Just a quick prod for reviews on this. I'm looking to open a vote > on > > > > Thursday if there are no objections. > > > > > > > > Thanks > > > > > > > > Tom > > > > > > > > > > > > > > > > > > Hey all, > > > > > > > > > > I'd like to open up the discussion on a KIP-734. This adds a new > > > > > OffsetSpec to AdminClient.listOffsets so that we can easily > determine > > > the > > > > > offset and timestamp of the message with the largest timestamp on a > > > > > partition. > > > > > > > > > > Please give it a look over and let me know what you think. > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-734%3A+Improve+AdminClient.listOffsets+to+return+timestamp+and+offset+for+the+record+with+the+largest+timestamp > > > > > > > > > > Thanks > > > > > > > > > > Tom > > > > > > > > > > > > > > >
