Great, I’ll ask again for a vote then on this KIP as it seems like all the 
concerns have been addressed.


> On 30 Jul 2024, at 16:00, David Arthur <mum...@gmail.com> wrote:
> 
> Omnia, thanks for the updates!
> 
>> Am happy to add section for throttling in this KIP if it is high concern
> or open a followup KIP for this once we already have the pagination in
> place. Which one do you suggest?
> 
> I'm okay leaving throttling for a future KIP. It might be useful to see the
> feature in action for a while before deciding if its necessary or the best
> way to approach it.
> 
> On Mon, Jul 22, 2024 at 9:23 AM Omnia Ibrahim <o.g.h.ibra...@gmail.com>
> wrote:
> 
>> 
>> Hi David, thanks for the feedback and sorry for taking long to respond as
>> I was off for a week.
>>> DA1: In "Public Interfaces" you say "max.request.pagination.size.limit"
>>> controls the max items to return by default. It's not clear to me if this
>>> is just a default, or if it is a hard limit. In KIP-966, this config
>> serves
>>> as a hard limit to prevent misconfigured or malicious clients from
>>> requesting too many resources. Can you clarify this bit?
>> 
>> `max.request.partition.size.limit` will be used in same way as KIP-966 I
>> just meant `max.request.partition.size.limit` will equal
>> `max.request.pagination.size.limit` by default unless it is specified
>> otherwise. I clarified this in the KIP now
>> 
>>> DA2: Is "ItemsLeftToFetch" accounting for authorization? If not, it could
>>> be considered a minor info leak.
>> 
>> This is a good point. Any of the requests still will count to what ACLs
>> and resources the authorised user is used by the client, the pagination
>> will not effect this.
>> In cases where the client is using user with wild ACLs I am assuming this
>> is okay and they have the right to see this info.
>> However am rethinking this now as it might not be that useful and we can
>> just relay on if the there is a next cursor or not to simplify the approach
>> similar to KIP-966. I have updated the KIP to reflect this.
>> 
>>> DA3: By splitting up results into pages, we are introducing the
>> possibility
>>> of inconsistency into these RPCs. For example, today MetadataRequest
>>> returns results from the same MetadataImage, so the response is
>> consistent.
>>> With the paging approach, it is possible (likely even) that different
>>> requests will be served from different MetadataImage-s, leading to
>>> inconsistencies. This can be even worse if paged requests go to different
>>> brokers that may be lagging behind in metadata propagation. BTW this
>> issue
>>> exists for KIP-966 as well. We don't necessarily need to solve this right
>>> away, but I think it's worth mentioning in the KIP.
>> 
>> I added a limitation section to the KIP to mention this. I also mentioned
>> it in the top section of public interfaces.
>> 
>>> DA4: Have we considered some generic throttling for paged requests? I
>>> expect it might be an issue if clients want to get everything and just
>> page
>>> through all of the results as quickly as possible.
>> I didn’t consider throttling for pagination requests as
>> Right now the only throttling AdminClient knows is throttling
>> TopicCreate/Delete which is different than pagination and might need it is
>> own conversation and KIP.
>> For example in the case of throttling and retries > timeouts, should
>> consider send back what we fetched so far and allow the operator to set the
>> cursor next time. If this is the case then we need to include cursor to all
>> the Option classes to these requests. Also Admin API for
>> DescribeTopicPartitionRequest in KIP-966 don’t provide Cursor as part of
>> DescribeTopicsOptions.
>> Also extending `controllerMutation` or should we separate the paging
>> throttling to its own quota
>> The only requests I think might actively scraped are `OffsetFetchRequest`,
>> `ListGroupsRequest`, `DescribeGroupsRequest` and
>> `ConsumerGroupDescribeRequest` to actively provide lag metrics/dashboards
>> to consumers. So there might be too many pages.
>> The rest of the requests mostly used during maintenance of the cluster or
>> incidents (specially the producer/txn requests) and operator of the cluster
>> need them to take a decision. The pagination just provides them with a way
>> to escape the timeout problem with large clusters. So am not sure adding
>> throttling during such time would be wise.
>> Am happy to add section for throttling in this KIP if it is high concern
>> or open a followup KIP for this once we already have the pagination in
>> place. Which one do you suggest?
>> 
>> Thanks
>> Omnia
>> 
>>> On 12 Jul 2024, at 14:56, David Arthur <mum...@gmail.com> wrote:
>>> 
>>> Hey Omnia, thanks for the KIP! I think this will be a really nice
>>> improvement for operators.
>>> 
>>> DA1: In "Public Interfaces" you say "max.request.pagination.size.limit"
>>> controls the max items to return by default. It's not clear to me if this
>>> is just a default, or if it is a hard limit. In KIP-966, this config
>> serves
>>> as a hard limit to prevent misconfigured or malicious clients from
>>> requesting too many resources. Can you clarify this bit?
>>> 
>>> DA2: Is "ItemsLeftToFetch" accounting for authorization? If not, it could
>>> be considered a minor info leak.
>>> 
>>> DA3: By splitting up results into pages, we are introducing the
>> possibility
>>> of inconsistency into these RPCs. For example, today MetadataRequest
>>> returns results from the same MetadataImage, so the response is
>> consistent.
>>> With the paging approach, it is possible (likely even) that different
>>> requests will be served from different MetadataImage-s, leading to
>>> inconsistencies. This can be even worse if paged requests go to different
>>> brokers that may be lagging behind in metadata propagation. BTW this
>> issue
>>> exists for KIP-966 as well. We don't necessarily need to solve this right
>>> away, but I think it's worth mentioning in the KIP.
>>> 
>>> DA4: Have we considered some generic throttling for paged requests? I
>>> expect it might be an issue if clients want to get everything and just
>> page
>>> through all of the results as quickly as possible.
>>> 
>>> Cheers,
>>> David
>>> 
>>> 
>>> On Tue, Jul 9, 2024 at 8:20 AM Omnia Ibrahim <o.g.h.ibra...@gmail.com>
>>> wrote:
>>> 
>>>> 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
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> --
>>> David Arthur
>> 
>> 
> 
> -- 
> David Arthur

Reply via email to