Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(AIP-217)!: change unreahcable pagination guidance #1392

Merged
merged 6 commits into from
Aug 5, 2024

Conversation

noahdietz
Copy link
Collaborator

@noahdietz noahdietz commented Jul 26, 2024

The paginated response guidance for unreachable population is changed significantly. Services must populate unreachable resources on a per-page basis i.e. as they are encountered while preparing a specific page fetch RPC response. As such, unreachable resources will be populated alongside results within a page. This is opposite of the previous guidance to populate unreachable only at the end of all paged content and being mutually exclusive from results. Services may continue to use the previous guidance within an API version for backwards compatibility purposes. Furthermore, unreachable must be an unordered list, and annotated as such.

Internal bug for doc(s) and details: http://b/350781494

Note: For observers, the reviewers of this PR are not the only reviewers of this change, they are simply the GitHub reviewers for the changes already approved by a broader set of reviewers.

@noahdietz noahdietz marked this pull request as ready for review July 29, 2024 22:13
@noahdietz noahdietz requested a review from a team as a code owner July 29, 2024 22:13
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo, and one suggestion for additional clarity (sorry for not bringing it up before) but otherwise fine.

aip/general/0217.md Outdated Show resolved Hide resolved
- Populate results that were previously considered unreachable on a following
page if their availability is restored and the paging parameters allow for
their inclusion.
- For example, if region `projects/example/locations/us-west1` was unavailable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth documenting that the ordering could be implicit here - if the service is documented to return resources in resource name order when the client doesn't specify anything, that's sufficient to prevent the newly-available resources from being returned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the scenario you are describing, but not sure what the suggestion is. I've included a bullet above this example to bring "documented default ordering" into scope of inclusion evaluation: 696b95d

Is that what you mean?

@noahdietz noahdietz merged commit d4d201c into aip-dev:master Aug 5, 2024
2 checks passed
@noahdietz noahdietz deleted the aip-217-paging branch August 5, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants