Thanks David, I've added the static method in the KIP.
For the timestamps I'd prefer to keep this as a future enhancement given the extra scope it would add to this KIP. Thanks Tom On Mon, May 17, 2021 at 9:12 AM David Jacot <dja...@confluent.io.invalid> wrote: > 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 > > > > > > > > > > > > > > > > > > > > >
