Thank you David, I started a new one and let’s close this. Best, Ziming Deng
> On Feb 21, 2022, at 5:31 PM, David Jacot <dja...@confluent.io.INVALID> wrote: > > Thanks for the update. LGTM. > > The vote thread looks weird. Could you check or > perhaps create a new one with the latest title? > > Best, > David > > On Fri, Feb 11, 2022 at 2:57 PM deng ziming <dengziming1...@gmail.com> wrote: >> >> David, Thank you for pointing out this. >> >> After some time, I sorted out the main configurations, the main difference >> are default.api.timeout.ms <http://default.api.timeout.ms/>, retries, >> request.timeout.ms <http://request.timeout.ms/>,metadata.max.age.ms >> <http://metadata.max.age.ms/>. Please take a look again when you are free. >> >>> On Feb 2, 2022, at 3:41 PM, David Jacot <dja...@confluent.io.INVALID> wrote: >>> >>> Hey, >>> >>> Thanks for updating the KIP. I think that there are a few more configs >>> which could >>> be used. e.g. all the network related configs - they are in both >>> consumer and admin >>> configurations as well. Is `session.timeout.ms` relevant in our >>> context? It does not >>> seem to be used when querying offsets. >>> >>> Regarding the usage of `request.timeout.ms` in the KafkaConsumer, it would >>> be >>> great if we could be clearer in the KIP. When I read "Will use >>> default.api.timeout.ms >>> instead of request.timeout.ms , This is a small bug and will be fixed >>> in a separate PR", >>> it is not clear what will be fixed where. We could say that the >>> KafkaConsumer is >>> inconsistent in its usage of the timeouts so the AdminClient will >>> behave slightly >>> differently. However, as it seems to be a bug, we will fix the Consumer and >>> we >>> can add a link to the Jira. >>> >>> It is important to get this section as clear as possible because this >>> is where questions >>> will be. >>> >>> Cheers, >>> David >>> >>> On Wed, Feb 2, 2022 at 7:23 AM deng ziming <dengziming1...@gmail.com> wrote: >>>> >>>> Hey David, >>>> I rechecked the ConsumerConfig and split the Compatibility section into 2 >>>> sub-sections >>>> regarding AdminClientConfig and ConsumerConfig, I also find a small bug >>>> that we use >>>> different timeout in `KafkaConsumer.beginningOffsets` and >>>> `KafkaConsumer.endOffsets`, >>>> I will fix this. >>>> >>>> Please help to review the KIP and the bug, thank you. >>>> >>>> Best, >>>> Ziming Deng >>>> >>>> >>>>> On Feb 1, 2022, at 6:31 PM, David Jacot <dja...@confluent.io.INVALID> >>>>> wrote: >>>>> >>>>> Thanks for the updated KIP. >>>>> >>>>> Regarding the compatibility section, I think that it would be >>>>> great if we could really stress that the configurations that >>>>> could reasonably be used to configure the tool are actually >>>>> all supported by the admin client. Regarding the retry mechanism, >>>>> the consumer will retry until `default.api.timeout.ms` is reached >>>>> and it seems that the admin client does the same by default. Do >>>>> you confirm this? >>>>> >>>>> Best, >>>>> David >>>>> >>>>> On Mon, Jan 31, 2022 at 11:12 AM David Jacot <dja...@confluent.io> wrote: >>>>>> >>>>>> Hey, >>>>>> >>>>>> Thanks for the KIP. I have a few comments: >>>>>> >>>>>> 1. I think that it would be better to name the KIP: "GetOffsetShell >>>>>> should support max-timestamp" >>>>>> or something like that as this is the initial intent of the change. >>>>>> >>>>>> 2. There is a typo: `OffsetSpce` -> `OffsetSpec`. >>>>>> >>>>>> 3. It would be great if we could further expand the compatibility >>>>>> section. It seems that the number >>>>>> of consumer configurations which could reasonably be used by >>>>>> `GetOffsetShell` is quite small (timeout, >>>>>> retries, etc.) and it seems that most of them (if not all) are >>>>>> supported by the admin client as well. I wonder >>>>>> if we could be explicit here and argue that the transition won't be >>>>>> noticed. I might be speculating here. >>>>>> >>>>>> 4. For completeness, I think that we should mention extending the >>>>>> consumer to support max-timestamp >>>>>> as well in the rejected alternatives. That would be another way to >>>>>> address the issue. However, I agree >>>>>> with you that using the admin client is better in the admin tools. >>>>>> >>>>>> Best, >>>>>> David >>>>>> >>>>>> On Sun, Jan 30, 2022 at 2:09 PM deng ziming <dengziming1...@gmail.com> >>>>>> wrote: >>>>>>> >>>>>>> Sorry all, I mean KIP-851 not KIP-734. >>>>>>> In KIP-734 we add a new OffsetSpec to AdminClient, in this KIP I just >>>>>>> extend this OffsetSpec to GetOffsetShell. >>>>>>> >>>>>>>> On Jan 30, 2022, at 6:29 PM, deng ziming <dengziming1...@gmail.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>> Hey all, I'm starting the voting on KIP-734. >>>>>>>> >>>>>>>> This supports a new OffsetSpec in GetOffsetShell so that we can easily >>>>>>>> determine the offset and timestamp of the message with the largest >>>>>>>> timestamp on a partition. This seems a simple change but replaced >>>>>>>> KafkaConsumer with AdminClient in GetOffsetShell. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Ziming Deng >>>>>>> >>>> >>
