Skip to content

Conversation

@stephenplusplus
Copy link
Contributor

@stephenplusplus stephenplusplus commented May 19, 2016

Fixes #1281

To Dos

This adds a setting for maxApiCalls on all methods that use the nextQuery style pagination, as well as Datastore's get and runQuery. Let me know if I missed any methods!

datastore.runQuery(query, { maxApiCalls: 1 }, function(err, results) {
  // 1 request was made.
  // If a 2nd request needed to be made, it wasn't.
  // This callback receives the results from 1 request.
});

Given the example above, is there a creative way to let the user know that we had to interrupt making API calls?

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 19, 2016
@stephenplusplus
Copy link
Contributor Author

After a quick @callmehiphop lint, I'll get going on tests.

@callmehiphop
Copy link
Contributor

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.

@stephenplusplus
Copy link
Contributor Author

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.

"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.

@stephenplusplus
Copy link
Contributor Author

@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 maxApiCalls property.

I added request interceptors to increment a numRequestsMade counter. This worked for a while, but then I ran into another hurdle; not all requests need to make multiple API calls, so limiting it to 1 and verifying only 1 request was sent isn't exactly testing what we think it is. If we get clever and set maxApiCalls to 0, we could technically get passing tests, though that seems like we're not really writing system tests at that point, because we're blocking all interactions with the "system".

@callmehiphop
Copy link
Contributor

@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?

@stephenplusplus
Copy link
Contributor Author

Sure, lmk if you think the unit tests I have written already could be improved. I'll add some systems.

@stephenplusplus
Copy link
Contributor Author

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.

This comment was marked as spam.

This comment was marked as spam.

@coveralls
Copy link

coveralls commented Jun 2, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling e062de3 on stephenplusplus:spp--1281 into 7ff0a5b on GoogleCloudPlatform:master.

@callmehiphop callmehiphop merged commit 4184a1a into googleapis:master Jun 2, 2016
sofisl pushed a commit that referenced this pull request Feb 3, 2026
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement. core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants