Re: [DISCUSS] KIP-776: Add Consumer#peek for debugging/tuning
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 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 wrote: > > > > Thanks for your feedback, Sagar, Boyang. > > > > I've added an additional API to take the Set 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 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? > 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 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 > > > > > > > > > > > > >
Re: [DISCUSS] KIP-776: Add Consumer#peek for debugging/tuning
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 wrote: > > Thanks for your feedback, Sagar, Boyang. > > I've added an additional API to take the Set 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 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? 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 > > 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 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 > > > > > > > > >
Re: [DISCUSS] KIP-776: Add Consumer#peek for debugging/tuning
Thanks for your feedback, Sagar, Boyang. I've added an additional API to take the Set 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 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? 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 > 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 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 > > > > > >
Re: [DISCUSS] KIP-776: Add Consumer#peek for debugging/tuning
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? 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 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 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 > > >
Re: [DISCUSS] KIP-776: Add Consumer#peek for debugging/tuning
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 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 >
[DISCUSS] KIP-776: Add Consumer#peek for debugging/tuning
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
