Hi Igor, 
> IS1: Without an indication of how many pages or items are left,
> it is impossible for the client to calculate progress.
> Having the cursor in the response just indicates whether there
> are more pages or not, it does not distinguish whether there is
> just one left, or a thousand more to go.
> Is it possible to include the indication, taking authorization
> into account?
I originally added this field after our offline discussion however after 
rethinking about it. I think there are a couple of misconceptions here (even 
with authorisation taken in account), 
1. There is no consistency guarantee between requests (similar to KIP-966), so 
this number not necessarily accurate between requests and it just will add to 
the inconsistency.
2. The operator would not manually provide the next cursor or fire the 
pagination between requests manually instead AdminClient will handle this 
similar to how we handled it in KIP-966 check 
`KafkaAdminClient::describeTopics`. So technically the operator will never see 
this value or adjust the ResponsePagingationLimit in between requests.

Based on this I don’t see this adding much value especially that AdminClient is 
the one handling the iterating through the pages.

> IS2: It is a shame that the pagination fields are different for
> each RPC. This allows for divergences or mistakes as e.g. the KIP
> currently includes GroupId under the Cursor for MetadataRequest.
> I was wondering if instead of identifying the next object in
> the cursor, we could instead indicate its index, always using
> an integer value. Or perhaps the page number, considering the
> value of ResponsePagingationLimit. Is is possible to have a standard
> way to deal with pagination, and use consistently for all RPCs?

Due to the inconsistency between requests I would say having the cursor as 
“item” instead of index would be better as adding resources between pagination 
requests that change the index might lead some odd cases as the request aren’t 
transparent regarding what resources we actually waiting for. For example, 
Next cursor : TopicPartition XX-0  has index 100
But by the time the next pagination request get fired someone else either 
a- created other topic partitions that pushed XX-0 down and take over index 100 
instead which mean this pagination request will be redundant as it might return 
same resources as the previous page (depend on how many topic partitions were 
added).
b- or someone deleted topic partition before index 100 making XX-0 and 
everything after index 100 jump to different position now fetching the next 
page with cursor 100 will lead to data loss in metadata and this case is much 
worse. 

We arguable can hit the similar issues with the proposed cursor but 
- the transparency about what you resources the cursor is targeting make the 
requests more clear. 
- I also don’t believe it lead to loss of metadata of old existing resources 
(as point b) and we only might miss new created resources if it happened to be 
before XX-0 if we sorted the resources or might return metadata for some 
recently deleted resources (and both cases existed without pagination as well).


> IS3: I don't quite understand the following rejected alternative:
> 
>> Make the pagination more generic by extending FieldSpec to include
>> pagination flag that indicate if a field will be used for pagination
>> or not. This would simplify the code however, it will be tricky to
>> use on fields like TopicPartition  which usually is defined using
>> two fields topic and partition index which usually represented as
>> nested field. We can consider this only for Response  to add an
>> extra flag to FieldSpec indicating that a field will have NextCursor
>> field but it is not clear what will be the usage of this except
>> making getting the next cursor maybe more generic to build but
>> arguable we can do this without touching FieldSpec.
> 
> The message specification format is only used in this project,
> so we can extend it or redefine it freely. If pagination is implemented
> in a consistent manner across RPCs, it would be valuable to have some
> high-level way to enable it in the RPC definition.
> Could you add a bit more detail about why that's not possible?

The reason why I think it wouldn’t work is the nested fields like what we have 
in `OffsetFetchResponse` or `DescribeLogDirsResponse` when we have nested 
levels like topic partitions within consumer group or topic partitions within 
logDir. 
Which what this section is mentioning. I’ll update it to make it a bit clearer.

>> it will be tricky to
>> use on fields like TopicPartition  which usually is defined using
>> two fields topic and partition index which usually represented as
>> Nested field

Thanks
Omnia

> On 4 Aug 2024, at 11:29, Igor Soarez <soa...@apache.org> wrote:
> 
> Hi Omnia,
> 
> Thanks for this KIP, this is a valuable improvement, and
> apologies for my late feedback on this.
> 
> IS1: Without an indication of how many pages or items are left,
> it is impossible for the client to calculate progress.
> Having the cursor in the response just indicates whether there
> are more pages or not, it does not distinguish whether there is
> just one left, or a thousand more to go.
> Is it possible to include the indication, taking authorization
> into account?
> 
> IS2: It is a shame that the pagination fields are different for
> each RPC. This allows for divergences or mistakes as e.g. the KIP
> currently includes GroupId under the Cursor for MetadataRequest.
> I was wondering if instead of identifying the next object in
> the cursor, we could instead indicate its index, always using
> an integer value. Or perhaps the page number, considering the
> value of ResponsePagingationLimit. Is is possible to have a standard
> way to deal with pagination, and use consistently for all RPCs?
> 
> IS3: I don't quite understand the following rejected alternative:
> 
>> Make the pagination more generic by extending FieldSpec to include
>> pagination flag that indicate if a field will be used for pagination
>> or not. This would simplify the code however, it will be tricky to
>> use on fields like TopicPartition  which usually is defined using
>> two fields topic and partition index which usually represented as
>> nested field. We can consider this only for Response  to add an
>> extra flag to FieldSpec indicating that a field will have NextCursor
>> field but it is not clear what will be the usage of this except
>> making getting the next cursor maybe more generic to build but
>> arguable we can do this without touching FieldSpec.
> 
> The message specification format is only used in this project,
> so we can extend it or redefine it freely. If pagination is implemented
> in a consistent manner across RPCs, it would be valuable to have some
> high-level way to enable it in the RPC definition.
> Could you add a bit more detail about why that's not possible?
> 
> Thanks,
> 
> --
> Igor
> 

Reply via email to