Hi Mickael, Thanks for your comments. I've added a use case into the KIP, and added Boyang's comment into rejected alternatives section.
Hope that makes sense and strengthens the motivation. If you have other suggestions, please let me know. Thank you. Luke On Mon, Oct 4, 2021 at 10:23 PM Mickael Maison <mickael.mai...@gmail.com> wrote: > Hi Luke, > > Thanks for the KIP. > > Can you clarify the use cases you have in mind exactly? This would > strengthen the motivation which I find a bit weak at the moment. > > As mentioned by Boyang, it's possible to achieve something similar > with the existing APIs, for example with poll/seek or with > listOffsets. Can you list them in the rejected alternatives section > and explain why they were rejected. > > Thanks > > > On Tue, Sep 21, 2021 at 9:47 AM Luke Chen <show...@gmail.com> wrote: > > > > Thanks for your feedback, Sagar, Boyang. > > > > I've added an additional API to take the Set<TopicPartition> as the > > partitions to fetch from. Good suggestion! > > I also updated the java doc in the KIP. > > > > And for the question that the behavior can also be achieved by using > manual > > offset commit + offset position rewind. That's true. > > But I have the same thoughts as Sagar, which is that, it's for advanced > > users. > > Another reason is for simplicity. If you've ever used the peek API from > > java collection (ex: Queue#peek), you should know what I'm talking about. > > When you have data in a queue, if you want to know what the first data is > > in the queue, you'd use peek(). You can also achieve it by remove() the > 1st > > element from queue, and then added it back to the right position, but I > > believe that's not what you'd do. > > > > Thank you. > > Luke > > > > > > On Tue, Sep 21, 2021 at 1:02 AM Sagar <sagarmeansoc...@gmail.com> wrote: > > > > > Thanks Luke for the KIP. I think it makes sense. > > > > > > @Boyang, > > > > > > While it is possible to get the functionality using manual offset > commit + > > > offset position rewind as you stated, IMHO it could still be a very > handy > > > addition to the APIs. The way I see it, manual offset commit + offset > > > position rewind is for slightly more advanced users and the addition of > > > peek() API would make it trivial to get the mentioned functionality. > > > > > > I agree to the point of adding a mechanism to fetch a more fine > grained set > > > of records. Maybe, add another API which takes a Set<TopicPartition>? > In > > > this case, we would probably need to add a behaviour to throw some > > > exception when a user tries to peek from a TopicPartition that he/she > isn't > > > subscribed to. > > > > > > nit: In the javadoc, this line => > > > > > > This method returns immediately if there are records available or > > > exception thrown. > > > > > > > > > should probably be => > > > > > > > > > This method returns immediately if there are no records available or > > > exception thrown. > > > > > > > > > Thanks! > > > > > > Sagar. > > > > > > > > > > > > > > > On Mon, Sep 20, 2021 at 4:22 AM Boyang Chen < > reluctanthero...@gmail.com> > > > wrote: > > > > > > > Thanks Luke for the KIP. > > > > > > > > I think I understand the motivation is to avoid affecting offset > > > positions > > > > of the records, but the feature could be easily realized on the user > side > > > > by using manual offset commit + offset position rewind. So the new > peek() > > > > function doesn't provide any new functionality IMHO, weakening the > > > > motivation a bit. > > > > > > > > Additionally, for the peek() case, I believe that users may want to > have > > > > more fine-grained exposure of records, such as from specific > partitions > > > > instead of getting random records. It's probably useful to define an > > > option > > > > handle class in the parameters to help clarify what specific records > to > > > be > > > > returned. > > > > > > > > Boyang > > > > > > > > On Sun, Sep 19, 2021 at 1:51 AM Luke Chen <show...@gmail.com> wrote: > > > > > > > > > Hi everyone, > > > > > > > > > > I'd like to discuss the following proposal to add Consumer#peek for > > > > > debugging/tuning. > > > > > > > > > > The main purpose for Consumer#peek is to allow users: > > > > > > > > > > 1. peek what records existed at broker side and not increasing > the > > > > > position offsets. > > > > > 2. throw exceptions when there is connection error existed > between > > > > > consumer and broker (or other exceptions will be thrown by > "poll") > > > > > > > > > > > > > > > detailed description can be found her: > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=188746244 > > > > > > > > > > > > > > > Any comments and feedback are welcomed. > > > > > > > > > > Thank you. > > > > > Luke > > > > > > > > > > > > >
