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