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

Default ec2 page size to 1000 #1971

Merged
merged 1 commit into from
May 13, 2016

Conversation

JordonPhillips
Copy link
Member

@JordonPhillips JordonPhillips commented May 9, 2016

If this value isn't set, EC2 will not paginate at all. Instead, they will
attempt to return all values to you at once. Naturally, this can cause
significant delay if you have a ton of resources.

Built off of #1970

cc @kyleknap @jamesls

@JordonPhillips JordonPhillips added the pr:needs-review This PR needs a review from a Member. label May 10, 2016
@@ -102,6 +103,7 @@ def awscli_initialize(event_handlers):
ec2_add_priv_launch_key)
register_parse_global_args(event_handlers)
register_pagination(event_handlers)
event_handlers.register('operation-args-parsed.ec2.*', set_max_results_default)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be doing this for every ec2 operation? Does every ec2 operation have the "paginate and give me a pagination token only if you gave me a page size to start"?

Copy link
Member Author

Choose a reason for hiding this comment

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

set_max_results checks if the function has pagination by looking for page_size, which is only present on functions that both have pagination enabled, and also have a limit key. At that point in the stack, I have no access to the pagination config so that's the only way to check. I could potentially manually compile a list of pagination operations, but then that would break whenever EC2 adds more.

@kyleknap
Copy link
Contributor

Looks fine. Had a couple of questions though that I would like answered.

@JordonPhillips
Copy link
Member Author

I rebased against develop since #1970 got merged. I also slightly simplified the if statement and clarified some of the logic.

@kyleknap
Copy link
Contributor

Hmm looks like you have some failing tests.

@JordonPhillips
Copy link
Member Author

Yeah, I think I may have messed up something when I rebased.

@jamesls jamesls added incorporating-feedback and removed pr:needs-review This PR needs a review from a Member. labels May 12, 2016
If this value isn't set, EC2 will not paginate at all. Instead, they
will attempt to return all values to you at once. Naturally, this can
cause significant delay if you have a ton of resources.
@JordonPhillips
Copy link
Member Author

I ended up not being able to use getattr(parsed_args, 'page_size', None) as Kyle suggested because I need to know if page size is present at all, and separately if it is None.

@jamesls
Copy link
Member

jamesls commented May 12, 2016

Change looks good to me. Looks like this is just pending a clean run through travis.

@JordonPhillips JordonPhillips added pr:needs-review This PR needs a review from a Member. and removed incorporating-feedback labels May 12, 2016
@JordonPhillips
Copy link
Member Author

👍

@kyleknap
Copy link
Contributor

Makes sense to me. 🚢

@JordonPhillips JordonPhillips merged commit 784e9fc into aws:develop May 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:needs-review This PR needs a review from a Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants