-
Notifications
You must be signed in to change notification settings - Fork 641
core (stream-router): allow setting maxApiCalls #1332
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
Conversation
cd5bf2c to
3b6d974
Compare
|
After a quick @callmehiphop lint, I'll get going on tests. |
|
Can we consolidate the retry code into a centralized place? The code looks good, my concern being that we've essentially duplicated it across files. |
|
Good idea! Pushed an update. |
| var requestsMade = 0; | ||
| var requestsToMake = -1; | ||
|
|
||
| options = options || {}; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
b8e73b8 to
2e15441
Compare
| "docs": "node ./scripts/docs.js", | ||
| "lint": "jshint lib/ system-test/ test/ && jscs lib/ system-test/ test/", | ||
| "test": "npm run docs && mocha test/*/*.js test/index.js test/docs.js", | ||
| "test": "npm run docs && mocha test/docs.js test/index.js test/*/*.js", |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
11b6632 to
e9c1888
Compare
|
@callmehiphop I'm currently struggling with how to approach system tests for this change. Since this is a modification to streamrouter, it affects many APIs. So, that means we would have to test every method using stream router again that it honors the I added request interceptors to increment a |
|
@stephenplusplus maybe it's a more appropriate place for unit tests then? I think as long as we have a couple system-tests, and really solid unit tests it should be ok. WDYT? |
|
Sure, lmk if you think the unit tests I have written already could be improved. I'll add some systems. |
|
System test for BigQuery added (other APIs aren't very compatible), docs published: http://stephenplusplus.github.io/gcloud-node/#/docs/master/gcloud |
|
|
||
| // Even though the request interceptor is called, the request can still be | ||
| // prevented if the `maxApiCalls` limit was reached. | ||
| numRequestsMade -= 1; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
* core (stream-router): allow setting maxApiCalls * move logic to iterator * iterator -> limiter * re-arrange stream-events use * tests. * lint * re-sort tests so overrides dont mess with docs * clean up maxApiCalls property * add system test for bigquery * docs * explain interceptors quirk
Fixes #1281
To Dos
This adds a setting for
maxApiCallson all methods that use thenextQuerystyle pagination, as well as Datastore'sgetandrunQuery. Let me know if I missed any methods!Given the example above, is there a creative way to let the user know that we had to interrupt making API calls?