Hi David, Thanks for having a look into this! > DJ1: Why do we want to migrate only a subset of the APIs vs migrating all > of them? For instance, there are DescribeGroups, ConsumerGroupDescribe, > etc. Do we have reasons not to migrate them too? I think that it would be > great to have a KIP that establishes the pattern for all the admin APIs.
No particular reason I only was trying to focus on the requests that cause the most pain ( as far as I know with Kafka). I don’t mind expand it to other requests in AdminClient as well. The other thing I was wondering about if max.request.partition.size.limit still make sense or should it be renamed (or have another one called max.request.pagingation.size.limit) if we going to expand to cover all Admin requests. I am leaning toward max.request.pagingation.size.limit and max.request.partition.size.limit can be set for now to max.request.partition.size.limit by default. I have updated the KIP now to cover more requests please have a second look and let me know if I missed any? > DJ2: I am not a fan of all the new parameters passed to the tools > (e.g. --partition-size-limit-per-response). I wonder if we could rather > have a config in the admin client to set the default page size for all > requests. If it going to be pattern to all requests I agree it should be an admin config then. Updated the KIP with this now. > DJ3: I assume that the Admin client will transparently handle the change. > It would be great to call it out in the compatibility section. Updated, Old clients will still have the old behaviour of no pagingation while all new versions will support this. Thanks Omnia > On 2 Jul 2024, at 10:43, David Jacot <dja...@confluent.io.INVALID> wrote: > > Hi Omnia, > > Thanks for the KIP. I agree that we should migrate admin APIs to the new > pattern. > > DJ1: Why do we want to migrate only a subset of the APIs vs migrating all > of them? For instance, there are DescribeGroups, ConsumerGroupDescribe, > etc. Do we have reasons not to migrate them too? I think that it would be > great to have a KIP that establishes the pattern for all the admin APIs. > > DJ2: I am not a fan of all the new parameters passed to the tools > (e.g. --partition-size-limit-per-response). I wonder if we could rather > have a config in the admin client to set the default page size for all > requests. > > DJ3: I assume that the Admin client will transparently handle the change. > It would be great to call it out in the compatibility section. > > Thanks, > David > > On Tue, Jul 2, 2024 at 11:17 AM Andrew Schofield <andrew_schofi...@live.com> > wrote: > >> Hi, >> Thanks for the response. Makes sense to me. Just one additional comment: >> >> AS5: The cursor for ListGroupsResponse is called `TransactionalCursor` >> which >> seems like a copy-paste mistake. >> >> Thanks, >> Andrew >> >>> On 30 Jun 2024, at 22:28, Omnia Ibrahim <o.g.h.ibra...@gmail.com> wrote: >>> >>> Hi Andrew thanks for having a look into the KIP >>> >>>> AS1: Besides topics, the most numerous resources in Kafka clusters in >> my experience >>>> are consumer groups. Would it be possible to extend the KIP to cover >> ListGroups while >>>> you’re in here? I’ve heard of clusters with truly vast numbers of >> groups. This is also >>>> potentially a sign of a misbehaving or poorly written clients. Getting >> a page of groups >>>> with a massive ItemsLeftToFetch would be nice. >>> Yes, I also had few experiences with large cluster where to list >> consumer groups can take up to 5min. I update the KIP to include this as >> well. >>> >>>> AS2: A tiny nit: The versions for the added fields are incorrect in >> some cases. >>> I believe I fixed all of them now >>> >>>> AS3: I don’t quite understand the cursor for >> OffsetFetchRequest/Response. >>>> It looks like the cursor is (topic, partition), but not group ID. Does >> the cursor >>>> apply to all groups in the request, or is group ID missing? >>> >>> I was thinking that the last one in the response will be the one that >> has the cursor while the rest will have null. But if we are moving >> NextCursour to the top level of the response then the cursor will need >> groupID. >>>> AS4: For the remaining request/response pairs, the cursor makes sense >> to me, >>>> but I do wonder whether `NextCursor` should be at the top level of the >> responses >>>> instead, like DescribeTopicPartitionsResponse. >>> >>> Updates the KIP to reflect this now. >>> >>> Let me know if you have any more feedback on this. >>> >>> Best >>> Omnia >>> >>>> On 27 Jun 2024, at 17:53, Andrew Schofield <andrew_schofi...@live.com> >> wrote: >>>> >>>> Hi Omnia, >>>> Thanks for the KIP. This is a really nice improvement for administering >> large clusters. >>>> >>>> AS1: Besides topics, the most numerous resources in Kafka clusters in >> my experience >>>> are consumer groups. Would it be possible to extend the KIP to cover >> ListGroups while >>>> you’re in here? I’ve heard of clusters with truly vast numbers of >> groups. This is also >>>> potentially a sign of a misbehaving or poorly written clients. Getting >> a page of groups >>>> with a massive ItemsLeftToFetch would be nice. >>>> >>>> AS2: A tiny nit: The versions for the added fields are incorrect in >> some cases. >>>> >>>> AS3: I don’t quite understand the cursor for >> OffsetFetchRequest/Response. >>>> It looks like the cursor is (topic, partition), but not group ID. Does >> the cursor >>>> apply to all groups in the request, or is group ID missing? >>>> >>>> AS4: For the remaining request/response pairs, the cursor makes sense >> to me, >>>> but I do wonder whether `NextCursor` should be at the top level of the >> responses >>>> instead, like DescribeTopicPartitionsResponse. >>>> >>>> Thanks, >>>> Andrew >>>> >>>>> On 27 Jun 2024, at 14:05, Omnia Ibrahim <o.g.h.ibra...@gmail.com> >> wrote: >>>>> >>>>> Hi everyone, I would like to start a discussion thread for KIP-1062 >>>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1062%3A+Introduce+Pagination+for+some+requests+used+by+Admin+API >>>>> >>>>> >>>>> Thanks >>>>> Omnia >> >> >>
