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