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

Don't error on duplicate pagination token #1748

Merged
merged 6 commits into from
Sep 27, 2022

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Sep 19, 2022

Motivation and Context

Some AWS operations, such as CloudWatch's GetLogEvents, respond with duplicate next page tokens so that infinite polling can be done (for use cases such as tailing a log). The SDK was previously erroring out in the paginator for this use case (see awslabs/aws-sdk-rust#620).

This PR adds a stop_on_duplicate_token option to the paginators, and removes the error. This solution is the same as what the Go V2 SDK is doing.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jdisanti jdisanti requested a review from a team as a code owner September 19, 2022 20:13
@jdisanti
Copy link
Collaborator Author

I currently have stop_on_duplicate_token defaulting to true, since it felt wrong to default to infinite looping. Not sure this is correct though. Will cross check what other SDKs are doing.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti
Copy link
Collaborator Author

It looks like Java V2 uses a customization to mark operations as ending pagination by returning a duplicate token. Go V2 requires the customer to specify a value for its StopOnDuplicateToken. The Rust version of this requires a default with the current fluent paginator builder design, so given the above, I'm going to keep it defaulting to true.

Comment on lines +151 to +152
/// Stop paginating when the service returns the same pagination token twice in a row.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

We should include docs on when this would be helpful to set to false:

When running status-checking operations or operations that you want to continually send in order to "watch" for the latest data (like AWS CloudWatch's GetLogEvents), set this to false. That way, you can continually call the paginator in order to receive new data as it becomes available.

@Velfi
Copy link
Contributor

Velfi commented Sep 26, 2022

It looks like Java V2 uses a customization to mark operations as ending pagination by returning a duplicate token. Go V2 requires the customer to specify a value for its StopOnDuplicateToken. The Rust version of this requires a default with the current fluent paginator builder design, so given the above, I'm going to keep it defaulting to true.

I think this is the right approach to take. Infinite looping is something that people shouldn't be able to accidentally trigger unless the docs are very clear about when it happens. I do think we should add some info on why infinite looping is sometimes helpful and guide people to pass false when relevant to their use case.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti force-pushed the jdisanti-paginators-duplicate-token branch from 0491fec to 1d330fb Compare September 27, 2022 00:56
@jdisanti jdisanti requested a review from a team as a code owner September 27, 2022 00:56
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti enabled auto-merge (squash) September 27, 2022 17:05
@jdisanti jdisanti merged commit 7b96ae4 into main Sep 27, 2022
@jdisanti jdisanti deleted the jdisanti-paginators-duplicate-token branch September 27, 2022 17:05
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.

2 participants