Hey, I will open a voting thread by end of this week if there is no more feedbacks.
Thanks Omnia > On 2 Jul 2024, at 17:25, Omnia Ibrahim <o.g.h.ibra...@gmail.com> wrote: > > 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 >>> >>> >>> >
