core: created streaming methods #1665

callmehiphop · 2016-10-03T19:02:43Z

This PR moves all streaming APIs to their own methods.

e.g.

bigquery.getJobs() -> bigquery.getJobsStream()

TODOS

Changes

All callback style methods are left intact, however as mentioned earlier, all streaming methods have been moved to their own methods. Here is a map of where streaming functionality has moved to.

BigQuery

BigQuery#query -> BigQuery#createQueryStream
BigQuery#getDatasets -> BigQuery#getDatasetsStream
BigQuery#getJobs -> BigQuery#getJobsStream
Dataset#query -> Dataset#createQueryStream
Dataset#getTables -> Dataset#getTablesStream
Job#getQueryResults -> Job#getQueryResultsStream
Table#query -> Table#createQueryStream
Table#getRows -> Table#createReadStream

Bigtable

Bigtable#getInstances -> Bigtable#getInstancesStream
Instance#getClusters -> Instance#getClustersStream
Instance#getTables -> Instance#getTablesStream
Table#getRows -> Table#createReadStream
Table#sampleRowKeys -> Table#sampleRowKeysStream

Compute

Compute#getAddresses -> Compute#getAddressesStream
Compute#getAutoscalers -> Compute#getAutoscalersStream
Compute#getDisks -> Compute#getDisksStream
Compute#getInstanceGroups -> Compute#getInstanceGroupsStream
Compute#getFirewalls -> Compute#getFirewallsStream
Compute#getHealthChecks -> Compute#getHealthChecksStream
Compute#getMachineTypes -> Compute#getMachineTypesStream
Compute#getNetworks -> Compute#getNetworksStream
Compute#getOperations -> Compute#getOperationsStream
Compute#getRegions -> Compute#getRegionsStream
Compute#getRules -> Compute#getRulesStream
Compute#getServices -> Compute#getServicesStream
Compute#getSnapshots -> Compute#getSnapshotsStream
Compute#getSubnetworks -> Compute#getSubnetworksStream
Compute#getVMs -> Compute#getVMsStream
Compute#getZones -> Compute#getZonesStream
InstanceGroup#getVMs -> InstanceGroup#getVMsStream
Network#getSubnetworks -> Network#getSubnetworksStream
Network#getFirewalls -> Network#getFirewallsStream
Region#getAddresses -> Region#getAddressesStream
Region#getOperations -> Region#getOperationsStream
Region#getRules -> Region#getRulesStream
Region#getSubnetworks -> Region#getSubnetworksStream
Zone#getAutoscalers -> Zone#getAutoscalersStream
Zone#getDisks -> Zone#getDisksStream
Zone#getInstanceGroups -> Zone#getInstanceGroupsStream
Zone#getMachineTypes -> Zone#getMachineTypesStream
Zone#getOperations -> Zone#getOperationsStream
Zone#getVMs -> Zone#getVMsStream

Datastore

Query#run -> Query#runStream
DatastoreRequest#get -> DatastoreRequest#createReadStream
DatastoreRequest#runQuery -> DatastoreRequest#runQueryStream

DNS

DNS#getZones -> DNS#getZonesStream
Zone#getChanges -> Zone#getChangesStream
Zone#getRecords -> Zone#getRecordsStream

Logging

Logging#getEntries -> Logging#getEntriesStream
Logging#getSinks -> Logging#getSinksStream
Log#getEntries -> Log#getEntriesStream

Prediction

Prediction#getModels -> Prediction#getModelsStream

PubSub

PubSub#getSubscriptions -> PubSub#getSubscriptionsStream
PubSub#getTopics -> PubSub#getTopicsStream
Topic#getSubscriptions -> Topic#getSubscriptionsStream

Resource

Resource#getProjects -> Resource#getProjectsStream

Storage

Storage#getBuckets -> Storage#getBucketsStream
Bucket#getFiles -> Bucket#getFilesStream

stephenplusplus · 2016-10-03T19:10:38Z

bigquery.getJobs() -> bigquery.getJobStream()

I think we should go with bigquery.getJobsStream() to avoid confusion.

packages/bigquery/src/dataset.js

+ *   .on('data', function(table) {
+ *     this.end();
+ *   });
+ */


packages/common/src/paginator.js

+ * @param {function} originalMethod - The original method.
+ * @return {function} wrapped - The wrapped method.
+ */
+paginator.wrap_ = function(originalMethod) {


packages/common/src/paginator.js

 *
- * streamRouter is used to extend `nextQuery`+callback methods with stream
- * functionality.
+ * paginator is used to auto paginated `nextQuery` methods as well as


packages/common/src/paginator.js

+ * @param {function} originalMethod - The original method.
+ * @return {function} wrapped - The wrapped method.
+ */
+paginator.wrap_ = function(originalMethod) {


callmehiphop · 2016-10-04T14:33:08Z

Class.prototype[originalMethodName + '_'] = originalMethod;

We can definitely do this, but it makes me a little nervous that it'll either require additional method renaming since the private version could exist or in the future it could lead to some confusion if we're overwriting private methods without realizing it.

stephenplusplus · 2016-10-04T14:35:22Z

I'm not too concerned about that, but you can always name it something more unique.

stephenplusplus · 2016-10-05T16:16:59Z

@callmehiphop @jgeewax wdyt re: bigquery.getJobStream() vs bigquery.getJobsStream()?

callmehiphop · 2016-10-05T16:25:22Z

IIRC we had settled on smushing the s's. I closest thing to written confirmation I could find was in #1142 (comment).

Personally I like the smushed naming better, and I don't think that is something that will confuse the user.

stephenplusplus · 2016-10-05T16:31:40Z

I meant "smushing" as in pushing them both together, not collapsing / some other word that's not clear. Basically, I prefer getJobsStream because we have a method called getJobs that this is the streaming version of. It makes it a no-brainer to know what to call when you want the stream mode. There's also getJobStream being singular ("this stream gets a job" vs "this stream gets multiple jobs"). Just curious about more thoughts on those points.

callmehiphop · 2016-10-07T04:32:18Z

@stephenplusplus I made a bunch of updates - I think this is in a pretty good place. PTAL!

stephenplusplus

Nice work, sir! I have a general comment that I didn't want to drop throughout, but I think in some places, this deviates from the alpha-sort pattern of methods. Please double check and try to keep it sorted in the source and tests as well, as it makes jumping into a bug fix / test cycle much easier. If the whole file was out of sort already, don't worry about taking on any extra work to clean it up, but I think most were sorted.

Other than that, just a few notes on naming / docs.

packages/bigquery/src/dataset.js

+ *
+ * See {module:bigquery#createQueryStream} for full documentation of this
+ * method.
+ */


packages/bigquery/src/dataset.js

+ *   .on('data', function(table) {
+ *     this.end();
+ *   });
+ */


packages/bigtable/src/table.js

+ * @return {stream}
 *
 * @example
+ * table.getRowsStream()


packages/bigtable/src/table.js

 /**
- * Get Row objects for the rows currently in your table.
+ * Get {module:bigtable/row} objects for the rows currently in your table as a
+ * readable object stream.


packages/common/src/paginator.js

 *
- * streamRouter is used to extend `nextQuery`+callback methods with stream
- * functionality.
+ * paginator is used to auto paginated `nextQuery` methods as well as


packages/common/src/paginator.js

 *
- *   search.query('done=true', function(err, results, nextQuery) {});
+ *   search.query('done=true', function(err, results, nextQuery) {
+ *     search.query(nextQuery, function(err, results, nextQuery) { ... });


packages/datastore/src/query.js

- * });
 */
-Query.prototype.run = function() {
+Query.prototype.stream = function() {


packages/datastore/src/request.js

+ * the end cursor from the previous query as the starting cursor for the next
+ * query. You can pass that object back to this method to see if more results
+ * exist.
+ *


scripts/link-common.js

+
+'use strict';
+
+var Module = require('./helpers').Module;


callmehiphop · 2016-10-07T17:35:05Z

@stephenplusplus I made some updates based on your feedback, PTAL!

stephenplusplus

👍

packages/bigquery/test/index.js

+      assert.strictEqual(bq.getDatasetsStream, 'getDatasets');
+      assert.strictEqual(bq.getJobsStream, 'getJobs');
+      assert.strictEqual(bq.createQueryStream, 'query');
+      assert.strictEqual(bq.getDatasetsStream, 'getDatasets');


callmehiphop · 2016-10-09T20:37:23Z

@stephenplusplus not trying to be annoying :) - but where do we stand on this?

stephenplusplus · 2016-10-09T20:47:04Z

Just a few comments we didn't discuss:

#1665 (comment)
#1665 (comment)
#1665 (comment)

Mobile now, but there may have been more PTAL

callmehiphop · 2016-10-10T03:18:04Z

Actually I resolved all of those, not sure why the comments are still in the feed.. maybe a quirk of starting a review?

stephenplusplus · 2016-10-10T13:13:32Z

Oh, that's frustrating. Well, while I dig back into the changes, it looks like there's a build bug: https://ci.appveyor.com/project/GoogleCloudPlatform/google-cloud-node/build/1.0.298/job/y5hsy2w05p6iqg6c

stephenplusplus · 2016-10-10T14:35:20Z

related: #1512

I think we should roll out a deprecation plan here:

Keep support for the old methods, but log a warning when it is used:

table.getRows() is deprecated starting in 1.0. Use table.createReadStream().

Change the docs for the old method to simply:

This method is deprecated! See {module:bigtable/table#createReadStream}.

We will git rid of the warnings in 1.0 stable, but for the beta, I think it's fair to print to the console.

Can you also make a map of old -> new methods, so we can include them all at once in the release notes?

// cc @richardkazuomiller

callmehiphop · 2016-10-10T14:48:39Z

it looks like there's a build bug

Not a bug per se, but we don't link common on CI, so the streamify() changes aren't present in any of the packages. You might have to fiddle with the tests locally to get them to pass.

Keep support for the old methods

Not sure how we are supposed to do that, the whole intention behind this is to add support for Promises, meaning when a callback is omitted to one of these original methods, we instead return a Promise.

Change the docs for the old method to simply:

This method is deprecated! See {module:bigtable/table#createReadStream}.

How do you want me to squeeze that in? Technically all the methods will still be usable with callbacks and promises.

Can you also make a map of old -> new methods, so we can include them all at once in the release notes?

Sure can.

stephenplusplus · 2016-10-10T14:52:00Z

Not a bug per se, but we don't link common on CI, so the streamify() changes aren't present in any of the packages.

Okay, what can we do to make the CI pass? Do we need to cut a release of changes in common?

Not sure how we are supposed to do that, the whole intention behind this is to add support for Promises, meaning when a callback is omitted to one of these original methods, we instead return a Promise.

I'm thinking of methods such as getRows -> createReadStream. This PR removes getRows, so I think we should instead add it as a wrapper that simply calls createReadStream.

How do you want me to squeeze that in? Technically all the methods will still be usable with callbacks and promises.

See above; these docs would be for the methods that were removed, that I think we should add back.

callmehiphop · 2016-10-10T14:56:29Z

Okay, what can we do to make the CI pass? Do we need to cut a release of changes in common?

Well yes, but this branch was being merged into the Promise support branch.. not master. I figured we'd get that branch working fine and then figure out a roll out plan later (break up commits, etc.)

See above; these docs would be for the methods that were removed, that I think we should add back.

I'll double check, but I'm pretty confident I didn't actually remove any methods, all the streaming APIs that I refactored had a non-streaming version that should still be intact.

stephenplusplus · 2016-10-10T14:59:02Z

Well yes, but this branch was being merged into the Promise support branch.. not master. I figured we'd get that branch working fine and then figure out a roll out plan later (break up commits, etc.)

Ah right, not a big deal then.

callmehiphop · 2016-10-10T15:05:16Z

I double checked and I can't find any methods that were outright deleted.

stephenplusplus · 2016-10-10T15:11:53Z

Okidoke, sorry to derail. No methods were removed, this only adds new methods with Stream() at the end of it. However, we skipped some implied methods, such as table.getRowsStream(), instead calling it table.createReadStream(). This isn't a deprecation in the normal sense, but is a deviation from our pattern. That means streaming users of table.getRows() will need to be aware they need to use table.createReadStream().

Can you list methods where we did that ^ and put them in the OP of this PR? I believe it may only be the example above and something from BigQuery?

callmehiphop · 2016-10-10T15:58:14Z

Since we needed a map of what functionality went where, I went ahead and just updated the overview with every method that has changed.

stephenplusplus · 2016-10-10T15:59:42Z

A beautiful sight. Thanks! imamerge.

core: created streaming methods

ba0145e

callmehiphop added core don't merge labels Oct 3, 2016

callmehiphop assigned stephenplusplus Oct 3, 2016

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 3, 2016

stephenplusplus reviewed Oct 3, 2016

View reviewed changes

callmehiphop commented Oct 3, 2016

View reviewed changes

updated streaming method descriptions with emitted type

9648c7b

stephenplusplus reviewed Oct 4, 2016

View reviewed changes

callmehiphop mentioned this pull request Oct 4, 2016

docs: update event emitters to use event tag #1667

Closed

update original method caching

15441f7

callmehiphop mentioned this pull request Oct 5, 2016

core: adding promise support #1670

Merged

callmehiphop added 2 commits October 6, 2016 16:20

pluralized all the stream method names

984cdf0

unit testing + linting

00e55ba

callmehiphop removed the don't merge label Oct 7, 2016

stephenplusplus reviewed Oct 7, 2016

View reviewed changes

renamed + alpha-sorted methods/tests

dc6fa56

stephenplusplus reviewed Oct 7, 2016

View reviewed changes

removed duplicate assertion

6f7bfb5

stephenplusplus merged commit ba75399 into googleapis:promise-support Oct 10, 2016

callmehiphop added a commit that referenced this pull request Oct 14, 2016

core: created streaming methods (#1665)

c491493

core: created streaming methods #1665

core: created streaming methods #1665

Uh oh!

Conversation

callmehiphop commented Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODOS

Changes

BigQuery

Bigtable

Compute

Datastore

DNS

Logging

Prediction

PubSub

Resource

Storage

Uh oh!

stephenplusplus commented Oct 3, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

callmehiphop commented Oct 4, 2016

Uh oh!

stephenplusplus commented Oct 4, 2016

Uh oh!

stephenplusplus commented Oct 5, 2016

Uh oh!

callmehiphop commented Oct 5, 2016

Uh oh!

stephenplusplus commented Oct 5, 2016

Uh oh!

callmehiphop commented Oct 7, 2016

Uh oh!

stephenplusplus left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

callmehiphop commented Oct 3, 2016 •

edited

Loading

stephenplusplus commented Oct 10, 2016 •

edited

Loading

stephenplusplus commented Oct 10, 2016 •

edited

Loading