Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Hi all,
I was looking at the OffsetFetchRequest in the context of KIP-848 and
I noticed something that I would like to discuss.
In the current implementation, when a group is specified multiple
times in the request, we ignore all occurrences but the last one. That
is because all groups are put in a Map so the last one wins. I find
this a bit strange because one case specifies multiple times the same
group but with different topics/partitions and he would get only a
response for the last one. This does not seem right to me. We could
return a response for each provided group (in the same order as in the
request). In practice, I suppose that no one uses the API this way and
note that it is not possible to do this with the admin client. What do
you think? Personally, I would consider this as a bug in the
implementation.
Best,
David
On Tue, Jul 12, 2022 at 7:43 PM Rajini Sivaram wrote:
>
> Hi all,
>
> Following the PR discussion on this KIP's Admin API PR (
> https://github.com/apache/kafka/pull/10964), we have changed the Admin API
> proposed in this KIP to use a separate ListConsumerGroupOffsetsSpec class
> to specify per-group spec including partitions for which offsets are
> fetched.
>
> The new APIs are:
>
>
>- ListConsumerGroupOffsetsResult listConsumerGroupOffsets(MapListConsumerGroupOffsetsSpec> groupSpecs, ListConsumerGroupOffsetsOptions
>options);
>- ListConsumerGroupOffsetsResult listConsumerGroupOffsets(MapListConsumerGroupOffsetsSpec> groupSpecs);
>
>
> Using a spec class rather than directly setting partitions in the map
> allows us to evolve the group spec more easily in future. It also avoids a
> Map with null values for the case where all partitions of the groups are
> being fetched.
>
> We have also replaced the proposed method that returns Map KafkaFuture>> in
> ListConsumerGroupOffsetsResult with a method that takes the groupId as
> argument to simplify the API. The new API is:
>
>- public KafkaFuture>
>partitionsToOffsetAndMetadata(String groupId);
>
>
> Please let me know if there are any concerns with these changes.
>
>
> Thank you,
>
> Rajini
>
>
> On Sat, Jul 3, 2021 at 12:18 AM Sanjana Kaundinya
> wrote:
>
> > Hello Everyone,
> >
> > I recently opened a PR for KIP-709 and it was pointed out that we still
> > need to come to a consensus on the Admin APIs. Specifically the concern was
> > around the `ListConsumerGroupOffsetsOptions` class. Currently that class
> > contains a List that acts as a filter for the specific
> > topic partitions the client wants to fetch offsets for a specific group.
> > Originally I had planned to extend this by adding a map of type Map > List> so when specifying topic partitions, the called could
> > specify it on a per group basis with the `ListConsumerGroupOffsetsOptions`
> > class. However it was noted that this is not the typical way that the
> > “Options” class is used for the requests. Instead they’re normally used as
> > additional options for the request, and generally the data for the request
> > is passed in as a constructor. Since we are taking the time to change this
> > API, might as well try to use some best practices and change how we use the
> > `ListConsumerGroupOffsetsOptions` class. I propose we change the
> > `listConsumerGroupOffsets` API as follows:
> >
> > Earlier it was proposed that the following will be the method signatures
> > we would add to Admin.java:
> >
> > default ListConsumerGroupOffsetsResult
> > listConsumerGroupOffsets(List groupIds) {
> >return listConsumerGroupOffsets(groupIds, new
> > ListConsumerGroupOffsetsOptions(groupIds));
> > }
> > ListConsumerGroupOffsetsResult listConsumerGroupOffsets(List
> > groupIds, ListConsumerGroupOffsetsOptions options);
> >
> > I propose we change the signatures to the following instead:
> >
> > default ListConsumerGroupOffsetsResult
> > listConsumerGroupOffsets(Map>
> > groupToTopicPartitions) {
> > return listConsumerGroupOffsets(groupToTopicPartitions, new
> > ListConsumerGroupOffsetOptions());
> > }
> > ListConsumerGroupOffsetsResult list listConsumerGroupOffsets(Map > List> groupToTopicPartitions,
> > ListConsumerGroupOffsetOptions options);
> >
> > This way we are transferring the data for the requests passed in as
> > parameters and this frees up the ListConsumerGroupOffsetOptions class to be
> > used in the future to apply different options to the request. Eventually we
> > will deprecate the single group listConsumerGroupOffsets method, and with
> > that the ListConsumerGroupOffsetsOptions method signature will also be
> > different, no longer storing the data for the topic partitions that we want
> > to retrieve offsets for. In essence, as part of this change, we will leave
> > the ListConsumerGroupOffsetsOptions unchanged, and eventually remove the
> > List we store there when we remove the deprecated single
> > listConsumerGroupOffsets method.
> >
> > Appreciate any feedback/discussion on this - thank you!
> >
> > Cheers,
> >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Hi all,
Following the PR discussion on this KIP's Admin API PR (
https://github.com/apache/kafka/pull/10964), we have changed the Admin API
proposed in this KIP to use a separate ListConsumerGroupOffsetsSpec class
to specify per-group spec including partitions for which offsets are
fetched.
The new APIs are:
- ListConsumerGroupOffsetsResult listConsumerGroupOffsets(Map groupSpecs, ListConsumerGroupOffsetsOptions
options);
- ListConsumerGroupOffsetsResult listConsumerGroupOffsets(Map groupSpecs);
Using a spec class rather than directly setting partitions in the map
allows us to evolve the group spec more easily in future. It also avoids a
Map with null values for the case where all partitions of the groups are
being fetched.
We have also replaced the proposed method that returns Map>> in
ListConsumerGroupOffsetsResult with a method that takes the groupId as
argument to simplify the API. The new API is:
- public KafkaFuture>
partitionsToOffsetAndMetadata(String groupId);
Please let me know if there are any concerns with these changes.
Thank you,
Rajini
On Sat, Jul 3, 2021 at 12:18 AM Sanjana Kaundinya
wrote:
> Hello Everyone,
>
> I recently opened a PR for KIP-709 and it was pointed out that we still
> need to come to a consensus on the Admin APIs. Specifically the concern was
> around the `ListConsumerGroupOffsetsOptions` class. Currently that class
> contains a List that acts as a filter for the specific
> topic partitions the client wants to fetch offsets for a specific group.
> Originally I had planned to extend this by adding a map of type Map List> so when specifying topic partitions, the called could
> specify it on a per group basis with the `ListConsumerGroupOffsetsOptions`
> class. However it was noted that this is not the typical way that the
> “Options” class is used for the requests. Instead they’re normally used as
> additional options for the request, and generally the data for the request
> is passed in as a constructor. Since we are taking the time to change this
> API, might as well try to use some best practices and change how we use the
> `ListConsumerGroupOffsetsOptions` class. I propose we change the
> `listConsumerGroupOffsets` API as follows:
>
> Earlier it was proposed that the following will be the method signatures
> we would add to Admin.java:
>
> default ListConsumerGroupOffsetsResult
> listConsumerGroupOffsets(List groupIds) {
>return listConsumerGroupOffsets(groupIds, new
> ListConsumerGroupOffsetsOptions(groupIds));
> }
> ListConsumerGroupOffsetsResult listConsumerGroupOffsets(List
> groupIds, ListConsumerGroupOffsetsOptions options);
>
> I propose we change the signatures to the following instead:
>
> default ListConsumerGroupOffsetsResult
> listConsumerGroupOffsets(Map>
> groupToTopicPartitions) {
> return listConsumerGroupOffsets(groupToTopicPartitions, new
> ListConsumerGroupOffsetOptions());
> }
> ListConsumerGroupOffsetsResult list listConsumerGroupOffsets(Map List> groupToTopicPartitions,
> ListConsumerGroupOffsetOptions options);
>
> This way we are transferring the data for the requests passed in as
> parameters and this frees up the ListConsumerGroupOffsetOptions class to be
> used in the future to apply different options to the request. Eventually we
> will deprecate the single group listConsumerGroupOffsets method, and with
> that the ListConsumerGroupOffsetsOptions method signature will also be
> different, no longer storing the data for the topic partitions that we want
> to retrieve offsets for. In essence, as part of this change, we will leave
> the ListConsumerGroupOffsetsOptions unchanged, and eventually remove the
> List we store there when we remove the deprecated single
> listConsumerGroupOffsets method.
>
> Appreciate any feedback/discussion on this - thank you!
>
> Cheers,
> Sanjana
> On May 14, 2021, 4:07 PM -0700, Sanjana Kaundinya ,
> wrote:
> > Hi Everyone,
> > I’ve begun working on this KIP now and found that another class will be
> needing public changes. I have updated the KIP to reflect this, so just
> wanted to update the dev list as well. You can find the updated KIP here:
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258
> > Thanks,
> > Sanjana
> > On Jan 27, 2021, 4:18 AM -0800, Thomas Scott , wrote:
> > > Hi Magnus,
> > >
> > > Thanks for the review, I've added //moved and explanation as requested.
> > >
> > > Thanks
> > >
> > > Tom
> > >
> > >
> > > On Wed, Jan 27, 2021 at 12:05 PM Magnus Edenhill
> wrote:
> > >
> > > > Hey Thomas,
> > > >
> > > > I'm late to the game.
> > > >
> > > > It looks like the "top level" ErrorCode moved from the top-level to
> the
> > > > Group array, which makes sense,
> > > > but it would be good if it was marked as // MOVED in the KIP and
> also a
> > > > note that top level errors that
> > > > are unrelated to the group will be returned as per-group errors.
> > > >
> > > >
> > > > Regards,
> > > > Magnus
> > > >
> > > >
> > > > Den tis
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Hello Everyone,
I recently opened a PR for KIP-709 and it was pointed out that we still need to
come to a consensus on the Admin APIs. Specifically the concern was around the
`ListConsumerGroupOffsetsOptions` class. Currently that class contains a
List that acts as a filter for the specific topic partitions
the client wants to fetch offsets for a specific group. Originally I had
planned to extend this by adding a map of type Map> so when specifying topic partitions, the called could
specify it on a per group basis with the `ListConsumerGroupOffsetsOptions`
class. However it was noted that this is not the typical way that the “Options”
class is used for the requests. Instead they’re normally used as additional
options for the request, and generally the data for the request is passed in as
a constructor. Since we are taking the time to change this API, might as well
try to use some best practices and change how we use the
`ListConsumerGroupOffsetsOptions` class. I propose we change the
`listConsumerGroupOffsets` API as follows:
Earlier it was proposed that the following will be the method signatures we
would add to Admin.java:
default ListConsumerGroupOffsetsResult listConsumerGroupOffsets(List
groupIds) {
return listConsumerGroupOffsets(groupIds, new
ListConsumerGroupOffsetsOptions(groupIds));
}
ListConsumerGroupOffsetsResult listConsumerGroupOffsets(List groupIds,
ListConsumerGroupOffsetsOptions options);
I propose we change the signatures to the following instead:
default ListConsumerGroupOffsetsResult listConsumerGroupOffsets(Map> groupToTopicPartitions) {
return listConsumerGroupOffsets(groupToTopicPartitions, new
ListConsumerGroupOffsetOptions());
}
ListConsumerGroupOffsetsResult list listConsumerGroupOffsets(Map> groupToTopicPartitions, ListConsumerGroupOffsetOptions
options);
This way we are transferring the data for the requests passed in as parameters
and this frees up the ListConsumerGroupOffsetOptions class to be used in the
future to apply different options to the request. Eventually we will deprecate
the single group listConsumerGroupOffsets method, and with that the
ListConsumerGroupOffsetsOptions method signature will also be different, no
longer storing the data for the topic partitions that we want to retrieve
offsets for. In essence, as part of this change, we will leave the
ListConsumerGroupOffsetsOptions unchanged, and eventually remove the
List we store there when we remove the deprecated single
listConsumerGroupOffsets method.
Appreciate any feedback/discussion on this - thank you!
Cheers,
Sanjana
On May 14, 2021, 4:07 PM -0700, Sanjana Kaundinya , wrote:
> Hi Everyone,
> I’ve begun working on this KIP now and found that another class will be
> needing public changes. I have updated the KIP to reflect this, so just
> wanted to update the dev list as well. You can find the updated KIP here:
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258
> Thanks,
> Sanjana
> On Jan 27, 2021, 4:18 AM -0800, Thomas Scott , wrote:
> > Hi Magnus,
> >
> > Thanks for the review, I've added //moved and explanation as requested.
> >
> > Thanks
> >
> > Tom
> >
> >
> > On Wed, Jan 27, 2021 at 12:05 PM Magnus Edenhill wrote:
> >
> > > Hey Thomas,
> > >
> > > I'm late to the game.
> > >
> > > It looks like the "top level" ErrorCode moved from the top-level to the
> > > Group array, which makes sense,
> > > but it would be good if it was marked as // MOVED in the KIP and also a
> > > note that top level errors that
> > > are unrelated to the group will be returned as per-group errors.
> > >
> > >
> > > Regards,
> > > Magnus
> > >
> > >
> > > Den tis 26 jan. 2021 kl 15:42 skrev Thomas Scott :
> > >
> > > > Thanks David I've updated it.
> > > >
> > > > On Tue, Jan 26, 2021 at 1:55 PM David Jacot wrote:
> > > >
> > > > > Great. That answers my question!
> > > > >
> > > > > Thomas, I suggest adding a Related/Future Work section in the
> > > > > KIP to link KIP-699 more explicitly.
> > > > >
> > > > > Thanks,
> > > > > David
> > > > >
> > > > > On Tue, Jan 26, 2021 at 1:30 PM Thomas Scott
> > > > > wrote:
> > > > >
> > > > > > Hi Mickael/David,
> > > > > >
> > > > > > I feel like the combination of these 2 KIPs gives the complete
> > > > solution
> > > > > > but they can be implemented independently. I have added a
> > > > > > description
> > > > and
> > > > > > links to KIP-699 to KIP-709 to this effect.
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > Tom
> > > > > >
> > > > > >
> > > > > > On Tue, Jan 26, 2021 at 11:44 AM Mickael Maison <
> > > > > mickael.mai...@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi Thomas,
> > > > > > > Thanks, the KIP looks good.
> > > > > > >
> > > > > > > David,
> > > > > > > I started working on exactly that a few weeks ago:
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-699%3A+FindCoordinators
> > > > > > > I hope
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Hi Everyone, I’ve begun working on this KIP now and found that another class will be needing public changes. I have updated the KIP to reflect this, so just wanted to update the dev list as well. You can find the updated KIP here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 Thanks, Sanjana On Jan 27, 2021, 4:18 AM -0800, Thomas Scott , wrote: > Hi Magnus, > > Thanks for the review, I've added //moved and explanation as requested. > > Thanks > > Tom > > > On Wed, Jan 27, 2021 at 12:05 PM Magnus Edenhill wrote: > > > Hey Thomas, > > > > I'm late to the game. > > > > It looks like the "top level" ErrorCode moved from the top-level to the > > Group array, which makes sense, > > but it would be good if it was marked as // MOVED in the KIP and also a > > note that top level errors that > > are unrelated to the group will be returned as per-group errors. > > > > > > Regards, > > Magnus > > > > > > Den tis 26 jan. 2021 kl 15:42 skrev Thomas Scott : > > > > > Thanks David I've updated it. > > > > > > On Tue, Jan 26, 2021 at 1:55 PM David Jacot wrote: > > > > > > > Great. That answers my question! > > > > > > > > Thomas, I suggest adding a Related/Future Work section in the > > > > KIP to link KIP-699 more explicitly. > > > > > > > > Thanks, > > > > David > > > > > > > > On Tue, Jan 26, 2021 at 1:30 PM Thomas Scott wrote: > > > > > > > > > Hi Mickael/David, > > > > > > > > > > I feel like the combination of these 2 KIPs gives the complete > > > solution > > > > > but they can be implemented independently. I have added a description > > > and > > > > > links to KIP-699 to KIP-709 to this effect. > > > > > > > > > > Thanks > > > > > > > > > > Tom > > > > > > > > > > > > > > > On Tue, Jan 26, 2021 at 11:44 AM Mickael Maison < > > > > mickael.mai...@gmail.com> > > > > > wrote: > > > > > > > > > > > Hi Thomas, > > > > > > Thanks, the KIP looks good. > > > > > > > > > > > > David, > > > > > > I started working on exactly that a few weeks ago: > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-699%3A+FindCoordinators > > > > > > I hope to complete my draft and start a discussion later on this > > > week. > > > > > > > > > > > > Thanks > > > > > > > > > > > > On Tue, Jan 26, 2021 at 10:06 AM David Jacot > > > > > wrote: > > > > > > > > > > > > > > Hi Thomas, > > > > > > > > > > > > > > Thanks for the KIP. Overall, the KIP looks good to me. > > > > > > > > > > > > > > I have only one question: The FindCoordinator API only supports > > > > > > > resolving one group id at the time. If we want to get the offsets > > > for > > > > > > > say N groups, that means that we have to first issue N > > > > FindCoordinator > > > > > > > requests, wait for the responses, group by coordinators, and then > > > > > > > send a OffsetFetch request per coordinator. I wonder if we should > > > > > > > also extend the FindCoordinator API to support resolving multiple > > > > > > > groups as well. This would make the implementation in the admin > > > > > > > client a bit easier and would ensure that we can handle multiple > > > > > > > groups end-to-end. Have you thought about this? > > > > > > > > > > > > > > Best, > > > > > > > David > > > > > > > > > > > > > > On Tue, Jan 26, 2021 at 10:13 AM Rajini Sivaram < > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Thomas, > > > > > > > > > > > > > > > > Thanks for the KIP, this is a useful addition for admin use > > > cases. > > > > It > > > > > > may > > > > > > > > be worth starting the voting thread soon if we want to get this > > > > into > > > > > > 2.8.0. > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 1:52 PM Thomas Scott > > > > > > > wrote: > > > > > > > > > > > > > > > > > Thanks Ismael, that's a lot better. I've updated the KIP with > > > > this > > > > > > > > > behaviour instead. > > > > > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 11:42 AM Ismael Juma < > > > ism...@juma.me.uk> > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Thanks for the KIP, Thomas. One question below: > > > > > > > > > > > > > > > > > > > > Should an Admin client with this new functionality be used > > > > > against > > > > > > an > > > > > > > > old > > > > > > > > > > > broker that cannot handle these requests then the methods > > > > will > > > > > > throw > > > > > > > > > > > UnsupportedVersionException as per the usual pattern. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Did we consider automatically falling back to the single > > > group > > > > id > > > > > > > > request > > > > > > > > > > if the more efficient one is not supported? > > > > > > > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott < > > > t...@confluent.io > > > > > > > > > > > wrote: > > > > > > >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Hi Magnus, Thanks for the review, I've added //moved and explanation as requested. Thanks Tom On Wed, Jan 27, 2021 at 12:05 PM Magnus Edenhill wrote: > Hey Thomas, > > I'm late to the game. > > It looks like the "top level" ErrorCode moved from the top-level to the > Group array, which makes sense, > but it would be good if it was marked as // MOVED in the KIP and also a > note that top level errors that > are unrelated to the group will be returned as per-group errors. > > > Regards, > Magnus > > > Den tis 26 jan. 2021 kl 15:42 skrev Thomas Scott : > > > Thanks David I've updated it. > > > > On Tue, Jan 26, 2021 at 1:55 PM David Jacot wrote: > > > > > Great. That answers my question! > > > > > > Thomas, I suggest adding a Related/Future Work section in the > > > KIP to link KIP-699 more explicitly. > > > > > > Thanks, > > > David > > > > > > On Tue, Jan 26, 2021 at 1:30 PM Thomas Scott wrote: > > > > > > > Hi Mickael/David, > > > > > > > > I feel like the combination of these 2 KIPs gives the complete > > solution > > > > but they can be implemented independently. I have added a description > > and > > > > links to KIP-699 to KIP-709 to this effect. > > > > > > > > Thanks > > > > > > > > Tom > > > > > > > > > > > > On Tue, Jan 26, 2021 at 11:44 AM Mickael Maison < > > > mickael.mai...@gmail.com> > > > > wrote: > > > > > > > > > Hi Thomas, > > > > > Thanks, the KIP looks good. > > > > > > > > > > David, > > > > > I started working on exactly that a few weeks ago: > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-699%3A+FindCoordinators > > > > > I hope to complete my draft and start a discussion later on this > > week. > > > > > > > > > > Thanks > > > > > > > > > > On Tue, Jan 26, 2021 at 10:06 AM David Jacot > > > > wrote: > > > > > > > > > > > > Hi Thomas, > > > > > > > > > > > > Thanks for the KIP. Overall, the KIP looks good to me. > > > > > > > > > > > > I have only one question: The FindCoordinator API only supports > > > > > > resolving one group id at the time. If we want to get the offsets > > for > > > > > > say N groups, that means that we have to first issue N > > > FindCoordinator > > > > > > requests, wait for the responses, group by coordinators, and then > > > > > > send a OffsetFetch request per coordinator. I wonder if we should > > > > > > also extend the FindCoordinator API to support resolving multiple > > > > > > groups as well. This would make the implementation in the admin > > > > > > client a bit easier and would ensure that we can handle multiple > > > > > > groups end-to-end. Have you thought about this? > > > > > > > > > > > > Best, > > > > > > David > > > > > > > > > > > > On Tue, Jan 26, 2021 at 10:13 AM Rajini Sivaram < > > > > rajinisiva...@gmail.com > > > > > > > > > > > > wrote: > > > > > > > > > > > > > Hi Thomas, > > > > > > > > > > > > > > Thanks for the KIP, this is a useful addition for admin use > > cases. > > > It > > > > > may > > > > > > > be worth starting the voting thread soon if we want to get this > > > into > > > > > 2.8.0. > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 1:52 PM Thomas Scott > > > > > wrote: > > > > > > > > > > > > > > > Thanks Ismael, that's a lot better. I've updated the KIP with > > > this > > > > > > > > behaviour instead. > > > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 11:42 AM Ismael Juma < > > ism...@juma.me.uk> > > > > > wrote: > > > > > > > > > > > > > > > > > Thanks for the KIP, Thomas. One question below: > > > > > > > > > > > > > > > > > > Should an Admin client with this new functionality be used > > > > against > > > > > an > > > > > > > old > > > > > > > > > > broker that cannot handle these requests then the methods > > > will > > > > > throw > > > > > > > > > > UnsupportedVersionException as per the usual pattern. > > > > > > > > > > > > > > > > > > > > > > > > > > > Did we consider automatically falling back to the single > > group > > > id > > > > > > > request > > > > > > > > > if the more efficient one is not supported? > > > > > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott < > > t...@confluent.io > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > I'm starting this thread to discuss KIP-709 to extend > > > > OffsetFetch > > > > > > > > > requests > > > > > > > > > > to accept multiple group ids. Please check out the KIP > > here: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 > > > > > > > > > > > > > > > > > > > > Any comments much appreciated. > > > > > > > > > > > > > > > > > > > > thanks, > > > > > > > > > > > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Hey Thomas, I'm late to the game. It looks like the "top level" ErrorCode moved from the top-level to the Group array, which makes sense, but it would be good if it was marked as // MOVED in the KIP and also a note that top level errors that are unrelated to the group will be returned as per-group errors. Regards, Magnus Den tis 26 jan. 2021 kl 15:42 skrev Thomas Scott : > Thanks David I've updated it. > > On Tue, Jan 26, 2021 at 1:55 PM David Jacot wrote: > > > Great. That answers my question! > > > > Thomas, I suggest adding a Related/Future Work section in the > > KIP to link KIP-699 more explicitly. > > > > Thanks, > > David > > > > On Tue, Jan 26, 2021 at 1:30 PM Thomas Scott wrote: > > > > > Hi Mickael/David, > > > > > > I feel like the combination of these 2 KIPs gives the complete > solution > > > but they can be implemented independently. I have added a description > and > > > links to KIP-699 to KIP-709 to this effect. > > > > > > Thanks > > > > > > Tom > > > > > > > > > On Tue, Jan 26, 2021 at 11:44 AM Mickael Maison < > > mickael.mai...@gmail.com> > > > wrote: > > > > > > > Hi Thomas, > > > > Thanks, the KIP looks good. > > > > > > > > David, > > > > I started working on exactly that a few weeks ago: > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-699%3A+FindCoordinators > > > > I hope to complete my draft and start a discussion later on this > week. > > > > > > > > Thanks > > > > > > > > On Tue, Jan 26, 2021 at 10:06 AM David Jacot > > > wrote: > > > > > > > > > > Hi Thomas, > > > > > > > > > > Thanks for the KIP. Overall, the KIP looks good to me. > > > > > > > > > > I have only one question: The FindCoordinator API only supports > > > > > resolving one group id at the time. If we want to get the offsets > for > > > > > say N groups, that means that we have to first issue N > > FindCoordinator > > > > > requests, wait for the responses, group by coordinators, and then > > > > > send a OffsetFetch request per coordinator. I wonder if we should > > > > > also extend the FindCoordinator API to support resolving multiple > > > > > groups as well. This would make the implementation in the admin > > > > > client a bit easier and would ensure that we can handle multiple > > > > > groups end-to-end. Have you thought about this? > > > > > > > > > > Best, > > > > > David > > > > > > > > > > On Tue, Jan 26, 2021 at 10:13 AM Rajini Sivaram < > > > rajinisiva...@gmail.com > > > > > > > > > > wrote: > > > > > > > > > > > Hi Thomas, > > > > > > > > > > > > Thanks for the KIP, this is a useful addition for admin use > cases. > > It > > > > may > > > > > > be worth starting the voting thread soon if we want to get this > > into > > > > 2.8.0. > > > > > > > > > > > > Regards, > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 1:52 PM Thomas Scott > > > wrote: > > > > > > > > > > > > > Thanks Ismael, that's a lot better. I've updated the KIP with > > this > > > > > > > behaviour instead. > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 11:42 AM Ismael Juma < > ism...@juma.me.uk> > > > > wrote: > > > > > > > > > > > > > > > Thanks for the KIP, Thomas. One question below: > > > > > > > > > > > > > > > > Should an Admin client with this new functionality be used > > > against > > > > an > > > > > > old > > > > > > > > > broker that cannot handle these requests then the methods > > will > > > > throw > > > > > > > > > UnsupportedVersionException as per the usual pattern. > > > > > > > > > > > > > > > > > > > > > > > > Did we consider automatically falling back to the single > group > > id > > > > > > request > > > > > > > > if the more efficient one is not supported? > > > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott < > t...@confluent.io > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > I'm starting this thread to discuss KIP-709 to extend > > > OffsetFetch > > > > > > > > requests > > > > > > > > > to accept multiple group ids. Please check out the KIP > here: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 > > > > > > > > > > > > > > > > > > Any comments much appreciated. > > > > > > > > > > > > > > > > > > thanks, > > > > > > > > > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Thanks David I've updated it. On Tue, Jan 26, 2021 at 1:55 PM David Jacot wrote: > Great. That answers my question! > > Thomas, I suggest adding a Related/Future Work section in the > KIP to link KIP-699 more explicitly. > > Thanks, > David > > On Tue, Jan 26, 2021 at 1:30 PM Thomas Scott wrote: > > > Hi Mickael/David, > > > > I feel like the combination of these 2 KIPs gives the complete solution > > but they can be implemented independently. I have added a description and > > links to KIP-699 to KIP-709 to this effect. > > > > Thanks > > > > Tom > > > > > > On Tue, Jan 26, 2021 at 11:44 AM Mickael Maison < > mickael.mai...@gmail.com> > > wrote: > > > > > Hi Thomas, > > > Thanks, the KIP looks good. > > > > > > David, > > > I started working on exactly that a few weeks ago: > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-699%3A+FindCoordinators > > > I hope to complete my draft and start a discussion later on this week. > > > > > > Thanks > > > > > > On Tue, Jan 26, 2021 at 10:06 AM David Jacot > > wrote: > > > > > > > > Hi Thomas, > > > > > > > > Thanks for the KIP. Overall, the KIP looks good to me. > > > > > > > > I have only one question: The FindCoordinator API only supports > > > > resolving one group id at the time. If we want to get the offsets for > > > > say N groups, that means that we have to first issue N > FindCoordinator > > > > requests, wait for the responses, group by coordinators, and then > > > > send a OffsetFetch request per coordinator. I wonder if we should > > > > also extend the FindCoordinator API to support resolving multiple > > > > groups as well. This would make the implementation in the admin > > > > client a bit easier and would ensure that we can handle multiple > > > > groups end-to-end. Have you thought about this? > > > > > > > > Best, > > > > David > > > > > > > > On Tue, Jan 26, 2021 at 10:13 AM Rajini Sivaram < > > rajinisiva...@gmail.com > > > > > > > > wrote: > > > > > > > > > Hi Thomas, > > > > > > > > > > Thanks for the KIP, this is a useful addition for admin use cases. > It > > > may > > > > > be worth starting the voting thread soon if we want to get this > into > > > 2.8.0. > > > > > > > > > > Regards, > > > > > > > > > > Rajini > > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 1:52 PM Thomas Scott > > wrote: > > > > > > > > > > > Thanks Ismael, that's a lot better. I've updated the KIP with > this > > > > > > behaviour instead. > > > > > > > > > > > > On Mon, Jan 25, 2021 at 11:42 AM Ismael Juma > > > wrote: > > > > > > > > > > > > > Thanks for the KIP, Thomas. One question below: > > > > > > > > > > > > > > Should an Admin client with this new functionality be used > > against > > > an > > > > > old > > > > > > > > broker that cannot handle these requests then the methods > will > > > throw > > > > > > > > UnsupportedVersionException as per the usual pattern. > > > > > > > > > > > > > > > > > > > > > Did we consider automatically falling back to the single group > id > > > > > request > > > > > > > if the more efficient one is not supported? > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott > > > > wrote: > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > I'm starting this thread to discuss KIP-709 to extend > > OffsetFetch > > > > > > > requests > > > > > > > > to accept multiple group ids. Please check out the KIP here: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 > > > > > > > > > > > > > > > > Any comments much appreciated. > > > > > > > > > > > > > > > > thanks, > > > > > > > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Great. That answers my question! Thomas, I suggest adding a Related/Future Work section in the KIP to link KIP-699 more explicitly. Thanks, David On Tue, Jan 26, 2021 at 1:30 PM Thomas Scott wrote: > Hi Mickael/David, > > I feel like the combination of these 2 KIPs gives the complete solution > but they can be implemented independently. I have added a description and > links to KIP-699 to KIP-709 to this effect. > > Thanks > > Tom > > > On Tue, Jan 26, 2021 at 11:44 AM Mickael Maison > wrote: > > > Hi Thomas, > > Thanks, the KIP looks good. > > > > David, > > I started working on exactly that a few weeks ago: > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-699%3A+FindCoordinators > > I hope to complete my draft and start a discussion later on this week. > > > > Thanks > > > > On Tue, Jan 26, 2021 at 10:06 AM David Jacot > wrote: > > > > > > Hi Thomas, > > > > > > Thanks for the KIP. Overall, the KIP looks good to me. > > > > > > I have only one question: The FindCoordinator API only supports > > > resolving one group id at the time. If we want to get the offsets for > > > say N groups, that means that we have to first issue N FindCoordinator > > > requests, wait for the responses, group by coordinators, and then > > > send a OffsetFetch request per coordinator. I wonder if we should > > > also extend the FindCoordinator API to support resolving multiple > > > groups as well. This would make the implementation in the admin > > > client a bit easier and would ensure that we can handle multiple > > > groups end-to-end. Have you thought about this? > > > > > > Best, > > > David > > > > > > On Tue, Jan 26, 2021 at 10:13 AM Rajini Sivaram < > rajinisiva...@gmail.com > > > > > > wrote: > > > > > > > Hi Thomas, > > > > > > > > Thanks for the KIP, this is a useful addition for admin use cases. It > > may > > > > be worth starting the voting thread soon if we want to get this into > > 2.8.0. > > > > > > > > Regards, > > > > > > > > Rajini > > > > > > > > > > > > On Mon, Jan 25, 2021 at 1:52 PM Thomas Scott > wrote: > > > > > > > > > Thanks Ismael, that's a lot better. I've updated the KIP with this > > > > > behaviour instead. > > > > > > > > > > On Mon, Jan 25, 2021 at 11:42 AM Ismael Juma > > wrote: > > > > > > > > > > > Thanks for the KIP, Thomas. One question below: > > > > > > > > > > > > Should an Admin client with this new functionality be used > against > > an > > > > old > > > > > > > broker that cannot handle these requests then the methods will > > throw > > > > > > > UnsupportedVersionException as per the usual pattern. > > > > > > > > > > > > > > > > > > Did we consider automatically falling back to the single group id > > > > request > > > > > > if the more efficient one is not supported? > > > > > > > > > > > > Ismael > > > > > > > > > > > > On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott > > wrote: > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > I'm starting this thread to discuss KIP-709 to extend > OffsetFetch > > > > > > requests > > > > > > > to accept multiple group ids. Please check out the KIP here: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 > > > > > > > > > > > > > > Any comments much appreciated. > > > > > > > > > > > > > > thanks, > > > > > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > > > > > > > >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Hi Mickael/David, I feel like the combination of these 2 KIPs gives the complete solution but they can be implemented independently. I have added a description and links to KIP-699 to KIP-709 to this effect. Thanks Tom On Tue, Jan 26, 2021 at 11:44 AM Mickael Maison wrote: > Hi Thomas, > Thanks, the KIP looks good. > > David, > I started working on exactly that a few weeks ago: > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-699%3A+FindCoordinators > I hope to complete my draft and start a discussion later on this week. > > Thanks > > On Tue, Jan 26, 2021 at 10:06 AM David Jacot wrote: > > > > Hi Thomas, > > > > Thanks for the KIP. Overall, the KIP looks good to me. > > > > I have only one question: The FindCoordinator API only supports > > resolving one group id at the time. If we want to get the offsets for > > say N groups, that means that we have to first issue N FindCoordinator > > requests, wait for the responses, group by coordinators, and then > > send a OffsetFetch request per coordinator. I wonder if we should > > also extend the FindCoordinator API to support resolving multiple > > groups as well. This would make the implementation in the admin > > client a bit easier and would ensure that we can handle multiple > > groups end-to-end. Have you thought about this? > > > > Best, > > David > > > > On Tue, Jan 26, 2021 at 10:13 AM Rajini Sivaram > > > wrote: > > > > > Hi Thomas, > > > > > > Thanks for the KIP, this is a useful addition for admin use cases. It > may > > > be worth starting the voting thread soon if we want to get this into > 2.8.0. > > > > > > Regards, > > > > > > Rajini > > > > > > > > > On Mon, Jan 25, 2021 at 1:52 PM Thomas Scott wrote: > > > > > > > Thanks Ismael, that's a lot better. I've updated the KIP with this > > > > behaviour instead. > > > > > > > > On Mon, Jan 25, 2021 at 11:42 AM Ismael Juma > wrote: > > > > > > > > > Thanks for the KIP, Thomas. One question below: > > > > > > > > > > Should an Admin client with this new functionality be used against > an > > > old > > > > > > broker that cannot handle these requests then the methods will > throw > > > > > > UnsupportedVersionException as per the usual pattern. > > > > > > > > > > > > > > > Did we consider automatically falling back to the single group id > > > request > > > > > if the more efficient one is not supported? > > > > > > > > > > Ismael > > > > > > > > > > On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott > wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > I'm starting this thread to discuss KIP-709 to extend OffsetFetch > > > > > requests > > > > > > to accept multiple group ids. Please check out the KIP here: > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 > > > > > > > > > > > > Any comments much appreciated. > > > > > > > > > > > > thanks, > > > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Hi Thomas, Thanks, the KIP looks good. David, I started working on exactly that a few weeks ago: https://cwiki.apache.org/confluence/display/KAFKA/KIP-699%3A+FindCoordinators I hope to complete my draft and start a discussion later on this week. Thanks On Tue, Jan 26, 2021 at 10:06 AM David Jacot wrote: > > Hi Thomas, > > Thanks for the KIP. Overall, the KIP looks good to me. > > I have only one question: The FindCoordinator API only supports > resolving one group id at the time. If we want to get the offsets for > say N groups, that means that we have to first issue N FindCoordinator > requests, wait for the responses, group by coordinators, and then > send a OffsetFetch request per coordinator. I wonder if we should > also extend the FindCoordinator API to support resolving multiple > groups as well. This would make the implementation in the admin > client a bit easier and would ensure that we can handle multiple > groups end-to-end. Have you thought about this? > > Best, > David > > On Tue, Jan 26, 2021 at 10:13 AM Rajini Sivaram > wrote: > > > Hi Thomas, > > > > Thanks for the KIP, this is a useful addition for admin use cases. It may > > be worth starting the voting thread soon if we want to get this into 2.8.0. > > > > Regards, > > > > Rajini > > > > > > On Mon, Jan 25, 2021 at 1:52 PM Thomas Scott wrote: > > > > > Thanks Ismael, that's a lot better. I've updated the KIP with this > > > behaviour instead. > > > > > > On Mon, Jan 25, 2021 at 11:42 AM Ismael Juma wrote: > > > > > > > Thanks for the KIP, Thomas. One question below: > > > > > > > > Should an Admin client with this new functionality be used against an > > old > > > > > broker that cannot handle these requests then the methods will throw > > > > > UnsupportedVersionException as per the usual pattern. > > > > > > > > > > > > Did we consider automatically falling back to the single group id > > request > > > > if the more efficient one is not supported? > > > > > > > > Ismael > > > > > > > > On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott wrote: > > > > > > > > > Hi, > > > > > > > > > > I'm starting this thread to discuss KIP-709 to extend OffsetFetch > > > > requests > > > > > to accept multiple group ids. Please check out the KIP here: > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 > > > > > > > > > > Any comments much appreciated. > > > > > > > > > > thanks, > > > > > > > > > > Tom > > > > > > > > > > > > > >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Hi Thomas, Thanks for the KIP. Overall, the KIP looks good to me. I have only one question: The FindCoordinator API only supports resolving one group id at the time. If we want to get the offsets for say N groups, that means that we have to first issue N FindCoordinator requests, wait for the responses, group by coordinators, and then send a OffsetFetch request per coordinator. I wonder if we should also extend the FindCoordinator API to support resolving multiple groups as well. This would make the implementation in the admin client a bit easier and would ensure that we can handle multiple groups end-to-end. Have you thought about this? Best, David On Tue, Jan 26, 2021 at 10:13 AM Rajini Sivaram wrote: > Hi Thomas, > > Thanks for the KIP, this is a useful addition for admin use cases. It may > be worth starting the voting thread soon if we want to get this into 2.8.0. > > Regards, > > Rajini > > > On Mon, Jan 25, 2021 at 1:52 PM Thomas Scott wrote: > > > Thanks Ismael, that's a lot better. I've updated the KIP with this > > behaviour instead. > > > > On Mon, Jan 25, 2021 at 11:42 AM Ismael Juma wrote: > > > > > Thanks for the KIP, Thomas. One question below: > > > > > > Should an Admin client with this new functionality be used against an > old > > > > broker that cannot handle these requests then the methods will throw > > > > UnsupportedVersionException as per the usual pattern. > > > > > > > > > Did we consider automatically falling back to the single group id > request > > > if the more efficient one is not supported? > > > > > > Ismael > > > > > > On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott wrote: > > > > > > > Hi, > > > > > > > > I'm starting this thread to discuss KIP-709 to extend OffsetFetch > > > requests > > > > to accept multiple group ids. Please check out the KIP here: > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 > > > > > > > > Any comments much appreciated. > > > > > > > > thanks, > > > > > > > > Tom > > > > > > > > > >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Hi Thomas, Thanks for the KIP, this is a useful addition for admin use cases. It may be worth starting the voting thread soon if we want to get this into 2.8.0. Regards, Rajini On Mon, Jan 25, 2021 at 1:52 PM Thomas Scott wrote: > Thanks Ismael, that's a lot better. I've updated the KIP with this > behaviour instead. > > On Mon, Jan 25, 2021 at 11:42 AM Ismael Juma wrote: > > > Thanks for the KIP, Thomas. One question below: > > > > Should an Admin client with this new functionality be used against an old > > > broker that cannot handle these requests then the methods will throw > > > UnsupportedVersionException as per the usual pattern. > > > > > > Did we consider automatically falling back to the single group id request > > if the more efficient one is not supported? > > > > Ismael > > > > On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott wrote: > > > > > Hi, > > > > > > I'm starting this thread to discuss KIP-709 to extend OffsetFetch > > requests > > > to accept multiple group ids. Please check out the KIP here: > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 > > > > > > Any comments much appreciated. > > > > > > thanks, > > > > > > Tom > > > > > >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Thanks Ismael, that's a lot better. I've updated the KIP with this behaviour instead. On Mon, Jan 25, 2021 at 11:42 AM Ismael Juma wrote: > Thanks for the KIP, Thomas. One question below: > > Should an Admin client with this new functionality be used against an old > > broker that cannot handle these requests then the methods will throw > > UnsupportedVersionException as per the usual pattern. > > > Did we consider automatically falling back to the single group id request > if the more efficient one is not supported? > > Ismael > > On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott wrote: > > > Hi, > > > > I'm starting this thread to discuss KIP-709 to extend OffsetFetch > requests > > to accept multiple group ids. Please check out the KIP here: > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 > > > > Any comments much appreciated. > > > > thanks, > > > > Tom > > >
Re: [DISCUSS] KIP-709: Extend OffsetFetch requests to accept multiple group ids
Thanks for the KIP, Thomas. One question below: Should an Admin client with this new functionality be used against an old > broker that cannot handle these requests then the methods will throw > UnsupportedVersionException as per the usual pattern. Did we consider automatically falling back to the single group id request if the more efficient one is not supported? Ismael On Mon, Jan 25, 2021 at 3:34 AM Thomas Scott wrote: > Hi, > > I'm starting this thread to discuss KIP-709 to extend OffsetFetch requests > to accept multiple group ids. Please check out the KIP here: > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173084258 > > Any comments much appreciated. > > thanks, > > Tom >
