Add pagination to open api spec for listing of namespaces, tables, views #9660
Conversation
open-api/rest-catalog-open-api.yaml
Outdated
| Clients will initiate the first paginated request by sending an empty `pageToken` e.g. `GET /tables?pageToken` or `GET /tables?pageToken=` | ||
| signaling to the service that the response should be paginated. | ||
|
|
||
| For servers that support pagination, they will recognize `pageToken` and return a `next-page-token` in response if there are more results available. |
There was a problem hiding this comment.
I still think the wording can be improved in this section to avoid using they to refer to servers. I did a previous suggestion which avoids using they
There was a problem hiding this comment.
additionally it's confusing to use pageToken and next-page-token (vs nextPageToken), so which one is correct here?
There was a problem hiding this comment.
I still think the wording can be improved in this section to avoid using they to refer to servers. I did a previous suggestion which avoids using they
+1
additionally it's confusing to use pageToken and next-page-token (vs nextPageToken), so which one is correct here?
I think we probably have to use next-page-token instead of nextPageToken, because it is the actual field name.
There was a problem hiding this comment.
+1 for using next-page-token, @nastra removed usage of they, let me know if everything looks good and we can land this?
|
@jackye1995 can we merge this? |
|
Looks like it's still pending review from @danielcweeks? Is there any further comments? |
…ews (apache#9660) * Add pagination to open api spec for listing of namespaces, tables, views * Rev for pagination in open api spec * Fix writing in pagination descriptions * Use opaque token * Rev on pagination pr writing * Rev on pagination pr writing py file * Rev on description for pageSize * revise pageToken description * minor rev and make int for pageSize * minor rev and remove emtpy val true for pageSize * use only PageToken instead of NextPageToken * address jack and nastra comments * address final comments * remove they and use min 1 for pageSize --------- Co-authored-by: Rahil Chertara <rchertar@amazon.com>
|
|
||
| Servers that support pagination will recognize `pageToken` and return a `next-page-token` in response if there are more results available. | ||
| After the initial request, it is expected that the value of `next-page-token` from the last response is used in the subsequent request. | ||
| Servers that do not support pagination will ignore `next-page-token` and return all results. |
There was a problem hiding this comment.
Servers that do not support pagination will ignore pageToken right? This should be the query param not the response field.
| PageToken: | ||
| description: | ||
| An opaque token which allows clients to make use of pagination for a list API (e.g. ListTables). | ||
| Clients will initiate the first paginated request by sending an empty `pageToken` e.g. `GET /tables?pageToken` or `GET /tables?pageToken=` |
There was a problem hiding this comment.
@rahil-c, in a spec you always want to be specific, not give options. Options make the spec more complicated. Here, only the second form should be added. The first form with no = is not recommended.
There was a problem hiding this comment.
Trying to remember what was decided during the community sync discussion, https://www.youtube.com/watch?v=uAQVGd5zV4I, starting at 45:04, seems like you said we should support both there. But maybe it's a misunderstanding of what Rahil meant there when he was talking about the first option of value not present.
There was a problem hiding this comment.
@rdblue @jackye1995 Yea I think based on the community sync I thought we were suppose to be flexible for what users can do hence I included the two options in the description.
I can remove the first form if needed.
…ews (apache#9660) * Add pagination to open api spec for listing of namespaces, tables, views * Rev for pagination in open api spec * Fix writing in pagination descriptions * Use opaque token * Rev on pagination pr writing * Rev on pagination pr writing py file * Rev on description for pageSize * revise pageToken description * minor rev and make int for pageSize * minor rev and remove emtpy val true for pageSize * use only PageToken instead of NextPageToken * address jack and nastra comments * address final comments * remove they and use min 1 for pageSize --------- Co-authored-by: Rahil Chertara <rchertar@amazon.com>
Dev List discussion thread around adding support for pagination in list namespaces, tables, and views: https://lists.apache.org/thread/lql05h02qtp8mgq74ovhb0ndd76ck4f3
Credit to @emkornfield for creating this google doc for listing all cases which this change is based off: https://docs.google.com/document/d/1bbfoLssY1szCO_Hm3_93ZcN0UAMpf7kjmpwHQngqQJ0/edit
cc @jackye1995 @rdblue @danielcweeks
Testing
Ran
make install, make lint, and python3 rest-catalog-open-api.pywithout issues.`