Skip to content

Conversation

@Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Feb 26, 2025

Problem

Follow up to discussion: #6672 (comment)

Currently the ec2 module does make paginated requests to the SDK endpoint. However, in order to show these results into the UI, we resolve them in entirety.
For the quick picks this is done here: https://github.com/aws/aws-toolkit-vscode/blob/4e568970e8d8646cf8e590d39d8891d68cebde84/packages/core/src/awsService/ec2/prompter.ts#L65
For the explorer this is done here: https://github.com/aws/aws-toolkit-vscode/blob/4e568970e8d8646cf8e590d39d8891d68cebde84/packages/core/src/awsService/ec2/explorer/ec2ParentNode.ts#L52

Solution

  • Refactor the EC2 client to process the instances within the pages they come in (i.e. not flattening them). This allows passing an AsyncCollection of pages to the quickPick.
  • Avoid flattening results in makePaginatedRequest by default.
  • Refactor client builder to be non-async, this causes some trickle-down refactoring, but allows makePaginatedRequest to not return a promise.
  • Simplify instance processing to make use of type predicates.
  • Remove mocks from Ec2Prompter tests, and refactor to test pagination behavior.

Alternative Solution

  • Repack the instances into artificial pages (i.e. a batched iterator). This is a little hacky since it involves us unpacking and repacking the instances. However, it does involve significantly less code changes.

Verification

In addition to updating the tests, we can add a logging statement when the quickPick loads an item from the AsyncIterable. This screenshot shows that loading two items was successful and triggered the AsyncIterable twice, as expected.
image

Future Work


  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

@Hweinstock Hweinstock changed the title fix(ec2): pagination fixes at the UI level. (WIP) fix(ec2): properly paginate instances in quickPick. Feb 27, 2025
@Hweinstock Hweinstock marked this pull request as ready for review February 27, 2025 18:08
@Hweinstock Hweinstock requested a review from a team as a code owner February 27, 2025 18:08
* A wrapper around Instance where we can safely assume InstanceId field exists.
*/
export interface SafeEc2Instance extends Instance {
export interface PatchedEc2Instance extends Instance {
Copy link
Contributor

Choose a reason for hiding this comment

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

would we have some other class for "unsafe"? or is that just an improvement that we are making in our wrapper class?

Suggested change
export interface PatchedEc2Instance extends Instance {
export interface Ec2Instance extends Instance {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an improvement here to make processing the responses easier. Avoids undefined checks in multiple places since we assert it once, and store that info in the type.

@Hweinstock Hweinstock merged commit 80c4367 into aws:feature/sdkv3 Feb 27, 2025
17 checks passed
@Hweinstock Hweinstock deleted the sdkv3/ec2-pagination branch February 27, 2025 21:40
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