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 >>>
