-
Notifications
You must be signed in to change notification settings - Fork 741
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
Use maximum permitted page size #295
Conversation
…e the maximum page size to minimize HTTP requests.
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
@@ -52,7 +52,7 @@ protected synchronized void populate() throws IOException { | |||
*/ | |||
public synchronized Map<String,GHRepository> getRepositories() throws IOException { | |||
Map<String,GHRepository> repositories = new TreeMap<String, GHRepository>(); | |||
for (GHRepository r : listRepositories()) { | |||
for (GHRepository r : listRepositories(100)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐜 Probably should amend the comment on line 75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be for listRepositories(int pageSize)
:
Unlike
getRepositories()
, this does not wait until all the repositories are returned.
So what do you think needs to be amended? listRepositories
overloads produce incremental results, whereas getRepositories
waits for the full list. I am not changing those semantics (that I know of!); I am just asking for larger batches, on the grounds that an organization with 300 repositories is better enumerated using 3 requests returning 100 entries each, rather than 10 requests returning 30 entries each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind -- I misread something while running through it.
Bonus of increasing request size: less likely to hit rate limits, probably.
🐝 with ant. |
@reviewbybees done |
@recena also suggests making 100 (the maximum) be the default size to begin with. I am not sure if the lower default is intentional for some reason. |
Use fibonacci? |
🐝 |
This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging. |
Pending GraphQL, performance is going to be bad, but do what we can.
@reviewbybees