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