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

refactor: Extract listResources utility function for handling pagination #1923

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

SanjayVas
Copy link
Member

@SanjayVas SanjayVas commented Nov 15, 2024

This also fixes an issue where EventGroupsServiceTest was waiting for a real 30s deadline to pass.
Also fixes the simulator issue of PR #1927:
The MC and EDP simulators were only reading the first page of results from ListEventGroups. This means that in environments with more EventGroups, not all test EventGroups would be read. This is exacerbated by #1916, as the default page size is now smaller.

@SanjayVas
Copy link
Member Author

CC @roaminggypsy

@wfa-reviewable
Copy link

This change is Reviewable

@SanjayVas SanjayVas force-pushed the sanjayvas-list-resources branch 4 times, most recently from 2734faf to 30d02a4 Compare November 18, 2024 22:41
@SanjayVas SanjayVas force-pushed the sanjayvas-list-resources branch 2 times, most recently from 120f1a2 to ddfb970 Compare November 18, 2024 23:22
Copy link
Contributor

@tristanvuong2021 tristanvuong2021 left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

@SanjayVas SanjayVas force-pushed the sanjayvas-list-resources branch 4 times, most recently from 668fa73 to 929c89a Compare November 21, 2024 20:43
Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 11 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/kotlin/org/wfanet/measurement/common/api/grpc/ListResources.kt line 66 at r2 (raw file):

      coroutineContext.ensureActive()

      val resourceList: ResourceList<T> = list(nextPageToken, remaining)

why not just do

val resourceList = list(nextPagetoken, remaining).take(remaining-resourceList.size)

Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/common/api/grpc/ListResources.kt line 66 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

why not just do

val resourceList = list(nextPagetoken, remaining).take(remaining-resourceList.size)

The items that are returned must match the page token that is returned so that the end caller can store the page token and use it for subsequent requests. Therefore it is an error to return fewer items than where the next page token would start from.

Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/kotlin/org/wfanet/measurement/common/api/grpc/ListResources.kt line 66 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The items that are returned must match the page token that is returned so that the end caller can store the page token and use it for subsequent requests. Therefore it is an error to return fewer items than where the next page token would start from.

I guess that's a difference in perceived usage. I think the function should cease to return a pageToken once the limit is reached.

Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/common/api/grpc/ListResources.kt line 66 at r2 (raw file):

I think the function should cease to return a pageToken once the limit is reached.

That is strictly incorrect, as it gives no signal that there are items beyond the limit. The implementation must return a next page token in this case.

Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/common/api/grpc/ListResources.kt line 66 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I think the function should cease to return a pageToken once the limit is reached.

That is strictly incorrect, as it gives no signal that there are items beyond the limit. The implementation must return a next page token in this case.

To clarify, this function is intended to be used to implement pagination, not just to call a paginated method. e.g. one List method impl calling another, or a UI that wants to display full pages of a specific size.

@SanjayVas SanjayVas force-pushed the sanjayvas-list-resources branch 3 times, most recently from e6320aa to 43fef9d Compare November 21, 2024 22:51
This also fixes an issue where EventGroupsServiceTest was waiting for a real 30s deadline to pass.
@SanjayVas SanjayVas force-pushed the sanjayvas-list-resources branch from 43fef9d to 1391cab Compare November 21, 2024 23:52
Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

…st page (#1927)

The MC and EDP simulators were only reading the first page of results
from ListEventGroups. This means that in environments with more
EventGroups, not all test EventGroups would be read. This is exacerbated
by #1916, as the default page size is now smaller.
@renjiezh
Copy link
Contributor

I accidentally merged #1927 into this branch instead of main branch.
The PR description has been updated to reflect #1927.

Copy link
Contributor

@renjiezh renjiezh left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 11 files at r1, 2 of 4 files at r2, 3 of 3 files at r3, 3 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

@renjiezh renjiezh merged commit 48561a1 into main Nov 22, 2024
6 of 7 checks passed
@renjiezh renjiezh deleted the sanjayvas-list-resources branch November 22, 2024 18:15
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.

5 participants