Convert to GAPIC #35

stephenplusplus · 2018-01-31T22:32:24Z

Fixes #23

Blockers

https://github.com/googleapis/nodejs-bigtable/pull/35/files#r170264277

Breaking Changes

- table.createFamily(name, rule)
+ table.createFamily(name, {
+   rule: rule,
+   gaxOptions: gaxOptions
+ })

- family.create(rule)
+ family.create({
+   rule: rule,
+   gaxOptions: gaxOptions
+ })

- row.create({ property: true })
+ row.create({
+   entry: {
+     property: true
+   },
+   gaxOptions: gaxOptions
+ })

- row.filter(filter, [onMatchEntries], [onNoMatchEntries])
+ row.filter(filter, {
+   onMatch: [onMatchEntries],
+   onNoMatch: [onNoMatchEntries],
+   gaxOptions: gaxOptions
+ })

- row.save('follows:jadams', 1)
+ row.save({
+   follows: {
+     jadams: 1
+   }
+ }, gaxOptions)

- bigtable.getInstances([ListInstancesRequest], callback)
+ bigtable.getInstances([gaxOptions], callback)

- bigtable.getInstancesStream()

- instance.getClusters([ListClustersRequest], callback)
+ instance.getClusters([gaxOptions], callback)

- instance.getClustersStream()

Todos

codecov · 2018-02-15T16:46:04Z

Codecov Report

Merging #35 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master    #35    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files           9      9            
  Lines        1004   1199   +195     
======================================
+ Hits         1004   1199   +195

Impacted Files	Coverage Δ
src/instance.js	`100% <100%> (ø)`	⬆️
src/family.js	`100% <100%> (ø)`	⬆️
src/index.js	`100% <100%> (ø)`	⬆️
src/cluster.js	`100% <100%> (ø)`	⬆️
src/row.js	`100% <100%> (ø)`	⬆️
src/table.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e680fa...b236b42. Read the comment docs.

stephenplusplus · 2018-02-15T16:51:34Z

@kolodny this is still a work in progress, but would you mind trying to run the "rows" system tests in system-test/bigtable.js? I currently have them skipped, but you can replace describe.skip with describe.only to isolate them.

The problem seems to be that where we received a string version of a Buffer before, we now receive a Buffer directly from GAPIC. For example, this is a row that is handled by ChunkTransformer, before and after:

// Before
{ chunks:
   [ { row_status: null,
       rowKey: 'YWxpbmNvbG4=',
       familyName: [Object],
       qualifier: [Object],
       timestampMicros: '1518711386138000',
       labels: [],
       value: 'AAAAAAAAAAE=',
       valueSize: 0,
       resetRow: false,
       commitRow: false },
     { row_status: null,
       rowKey: '',
       familyName: null,
       qualifier: [Object],
       timestampMicros: '1518711386138000',
       labels: [],
       value: 'AAAAAAAAAAE=',
       valueSize: 0,
       resetRow: false,
       commitRow: false },
     { row_status: 'commitRow',
       rowKey: '',
       familyName: null,
       qualifier: [Object],
       timestampMicros: '1518711386138000',
       labels: [],
       value: 'AAAAAAAAAAE=',
       valueSize: 0,
       resetRow: false,
       commitRow: true } ],
  lastScannedRowKey: '' }

// After
{ chunks:
   [ { labels: [],
       rowKey: <Buffer 61 6c 69 6e 63 6f 6c 6e>,
       familyName: [Object],
       qualifier: [Object],
       timestampMicros: '1518711494344000',
       value: <Buffer 00 00 00 00 00 00 00 01>,
       valueSize: 0 },
     { labels: [],
       rowKey: [],
       familyName: null,
       qualifier: [Object],
       timestampMicros: '1518711494344000',
       value: <Buffer 00 00 00 00 00 00 00 01>,
       valueSize: 0 },
     { labels: [],
       rowKey: [],
       familyName: null,
       qualifier: [Object],
       timestampMicros: '1518711494344000',
       value: <Buffer 00 00 00 00 00 00 00 01>,
       valueSize: 0,
       commitRow: true,
       rowStatus: 'commitRow' } ],
  lastScannedRowKey: [] }

There are lots of methods that seem to handle converting between strings and Buffers, and I'm wondering if these are still necessary, or if this new response type is problematic for other reasons.

Please take a look and feel free to make any corrections necessary. Your help will surely be quicker and more accurate. Let me know if you have any questions about the new GAPIC layer or run into other issues.

sduskis · 2018-02-15T16:56:09Z

@ajaaym, can you also please take a look?

src/table.js

-        currentRetryAttempt: numRequestsMade,
-      },
-    };
+    // @todo Figure out how to do this in gapic.


src/v2/bigtable_instance_admin_client.js

@@ -127,6 +127,11 @@ class BigtableInstanceAdminClient {
        'nextPageToken',
        'appProfiles'
      ),
+      listClusters: new gax.PageDescriptor(


stephenplusplus · 2018-02-27T00:16:24Z

I've updated the PR as far as I think I can go without some help:

@kolodny Please verify retry logic is working. The system-test/mutate-rows.js file is hard to work with, since it is faking timers. Could someone please take a look? I've updated it to override the new request methods, but it's still having trouble.
@alexander-fenster The generated file had to be changed. Can we fix this in the generator? (Convert to GAPIC #35 (comment))

kolodny · 2018-02-28T20:38:13Z

@stephenplusplus I believe you needed to fake out slightly more than just setTimeout. To be honest, I'm not sure why the read-rows work. I have a proof of concept at kolodny@eaff5c3

I made the patching a little more aggressive but I'm not sure that's the best approach either. We can always completely remove the fake timers but we'd need to set the mocha timeout to at least a minute to take into account the exponential backoffs.

Let me know if that unblocks the system tests.

stephenplusplus · 2018-02-28T21:30:20Z

The patch concept is fine with me. Although, could you write a Windows-compliant one? I'm not sure exactly what the resulting patched file is meant to look like.

kolodny · 2018-03-02T14:23:09Z

Sure f6d6dea...kolodny:pr-35 has a cross platform patching mechanism. It just replaces the file (wherever it finds it) with the patched version

kolodny · 2018-03-02T14:25:12Z

Patching is probably the wrong term for it now since we're overwriting the file completely, so feel free to rename it to "fixed" or something similar.

stephenplusplus · 2018-03-02T14:44:53Z

Thanks for doing that. I've tried applying the changes from that commit, but end up with 102 errors from the system test run that look like:

  102) Bigtable/Table
       createReadStream
         does the previous 5 things in one giant test case:
     TypeError: processNextTick is not a function
      at resume (node_modules\readable-stream\lib\_stream_readable.js:783:5)
      at DestroyableTransform.Readable.resume (node_modules\readable-stream\lib\_stream_readable.js:775:5)
      at DestroyableTransform.Readable.on (node_modules\readable-stream\lib\_stream_readable.js:745:53)
      at DestroyableTransform.Readable.pipe (node_modules\readable-stream\lib\_stream_readable.js:615:7)
      at pipe (node_modules\pump\index.js:56:15)
      at Array.reduce (<anonymous>)
      at pump (node_modules\pump\index.js:79:11)
      at Pumpify.setPipeline (node_modules\pumpify\index.js:39:5)
      at new Pumpify (node_modules\pumpify\index.js:15:30)
      at Function.Pumpify [as obj] (node_modules\pumpify\index.js:13:44)
      at makeNewRequest (src\table.js:511:31)
      at Table.createReadStream (src\table.js:540:3)
      at Context.it (system-test\read-rows.js:128:15)

I've confirmed that the patched file does overwrite the originals inside the node_modules directory.

kolodny · 2018-03-02T14:54:06Z

Strange it works for me. What is node_modules\readable-stream\lib\_stream_readable.js line 783 ? Is it. Is is processNextTick(resume_, stream, state); if so does this line exist in that file too:

var processNextTick = require('process-nextick-args');

And if that is also found, can you try just doing something simple with process-nextick-args like running a script as follows:

var processNextTick = require('process-nextick-args');
processNextTick(console.log, 'test');

stephenplusplus · 2018-03-02T14:59:34Z

Is is processNextTick(resume_, stream, state);

Yep.

if so does this line exist in that file too:
var processNextTick = require('process-nextick-args');

Yep.

can you try just doing something simple with process-nextick-args like running a script

> require('process-nextick-args')(console.log, 'test')
undefined
> test

I noticed that the top of _stream_readable.js requires:

var processNextTick = require('process-nextick-args').nextTick;

But our patch doesn't export nextTick. My patched file for process-nextick-args/index.js is:

module.exports = function() {
  return process.nextTick.apply(this, arguments);
};

stephenplusplus · 2018-03-02T15:00:13Z

Oh sorry, maybe I answered question 2 incorrectly. It does require the module, but it looks for the nextTick property.

kolodny · 2018-03-02T15:32:06Z

Interesting, it looks like the process-nextick-args fix may have finally been published. Can you try deleting your node_modules folder and try again once again. If that fails, try deleting the node_modules and patch folders, remove the presystem-test line in package.json and try again

stephenplusplus · 2018-03-02T16:10:27Z

I deleted the patching pieces, although I can't be sure that was a good move. The system tests began to pass again, up until system-test/mutate-rows.js. They simply end abruptly without any error output. What happens locally when you rm -rf node_modules, npm install, and npm run system-test?

kolodny · 2018-03-02T16:15:19Z

Weird, on my system I only get the "bad" version of it. Try changing the patch code to something like this:

const fs = require('fs');
const glob = require('glob');

const patched = `${__dirname}/patched-process-nextick-args.js`;
glob.sync('./**/process-nextick-args/package.json').forEach(file => {
  const majorVersion = require(file).version.slice(0, 1);
  if (majorVersion <= 1) {
    fs.copyFileSync(patched, file);
  }
});

stephenplusplus · 2018-03-02T17:48:26Z

I had to re-work the script a bit to work, but the same early exiting without any output happens for me.

revised script:

const fs = require('fs');
const glob = require('glob');
const path = require('path');

const patched = `${__dirname}/patched-process-nextick-args.js`;
glob.sync('./**/process-nextick-args').forEach(pnaDirectoryPath => {
  const packageJson = require(path.join('../', pnaDirectoryPath, 'package.json'));
  const majorVersion = parseInt(packageJson.version[0], 10);

  if (majorVersion <= 1) {
    fs.copyFileSync(patched, path.join(pnaDirectoryPath, 'index.js'));
  }
});

This reverts commit a6a3c8c.

This reverts commit d34724c.

stephenplusplus · 2018-03-12T20:24:43Z

Updated to remove the pagination behavior from bigtable.getInstances() and instance.getClusters(). See the additions to the diff section from my first post.

This should be ready to go. ✈️

alexander-fenster

I'm OK, let's wait for approvals of @sduskis or @kolodny.

kolodny

I left a couple of comments but those shouldn't block this PR.

Look good to me!

src/cluster.js

+ * //-
+ * // If the callback is omitted, we'll return a Promise.
+ * //-
+ * cluster.create().then(function(data) {


src/index.js

-        baseUrl: adminBaseUrl,
-        path: 'google/bigtable/admin/v2/bigtable_table_admin.proto',
-        service: 'bigtable.admin.v2',
+  var options_ = extend(


src/instance.js

+    },
+    function(err, resp) {
+      if (err) {
+        callback(err);


src/row.js

+
+  this.getMetadata(gaxOptions, function(err) {
+    if (err) {
+      if (err instanceof RowError) {


src/table.js

@@ -1036,8 +1136,8 @@ Table.prototype.mutate = function(entries, callback) {
  var entryToIndex = new Map(entries.map((entry, index) => [entry, index]));
  var mutationErrorsByEntryIndex = new Map();

-  function onBatchResponse(previousNumRequestsMade, err) {
-    if (previousNumRequestsMade === numRequestsMade && err) {


src/row.js

@@ -275,21 +249,21 @@ Row.formatFamilies_ = function(families, options) {
 *   var apiResponse = data[0];
 * });
 */
-Row.prototype.create = function(entry, callback) {
+Row.prototype.create = function(options, callback) {


test/filter.js

@@ -42,8 +42,7 @@ describe('Bigtable/Filter', function() {
  });

  afterEach(function() {
-    sinon.restore();


sduskis · 2018-03-13T13:53:16Z

LGTM as well. It looks like table.createFamily()'s parameters changed a bit, and there might be some other changes lurking. I'm ok with the changes being committed, as long as we document them in some sort of release notes.

stephenplusplus · 2018-03-13T14:05:19Z

@sduskis all of the breaking changes are noted in the opening post (#35 (comment)) -- let me know if any of those look like a problem.

system-test/read-rows.js

-        );
-        assert.deepStrictEqual(requestedOptions, test.request_options);
+
+        setImmediate(function() {


stephenplusplus added status: blocked Resolving the issue is dependent on other work. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jan 31, 2018

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 31, 2018

ghost assigned stephenplusplus Jan 31, 2018

stephenplusplus removed the status: blocked Resolving the issue is dependent on other work. label Feb 14, 2018

stephenplusplus force-pushed the spp--23 branch from 8ab32e1 to aa42d08 Compare February 15, 2018 16:46

stephenplusplus force-pushed the spp--23 branch from aa42d08 to 30dd868 Compare February 20, 2018 00:18

stephenplusplus commented Feb 20, 2018

View reviewed changes

src/table.js Outdated

currentRetryAttempt: numRequestsMade,

},

};

// @todo Figure out how to do this in gapic.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ajaaym mentioned this pull request Feb 20, 2018

fix#47 Respect decode: false option when cell value is split between chunks #48

Merged

3 tasks

kolodny mentioned this pull request Feb 20, 2018

Improve transform perf #46

Merged

2 tasks

stephenplusplus force-pushed the spp--23 branch from 30dd868 to 970aa04 Compare February 21, 2018 18:48

stephenplusplus added the status: blocked Resolving the issue is dependent on other work. label Feb 23, 2018

stephenplusplus commented Feb 23, 2018

View reviewed changes

stephenplusplus force-pushed the spp--23 branch from 7b06281 to e6a7b39 Compare February 23, 2018 21:09

stephenplusplus and others added 12 commits March 12, 2018 16:10

Remove duplicate code

edde5d7

Fixed broken mutate-rows tests on GAPIC branch

5f8ae98

Remove patches

f27d704

Revert "Remove patches"

5e58cd1

This reverts commit a6a3c8c.

Fix patch & use new test

5139d7e

Get windows to run test

404b5a4

Fixed broken mutate-rows tests on GAPIC branch nextickv2

e2d4429

Revert "Get windows to run test"

ed965f0

This reverts commit d34724c.

Tests work!

7fe2fa0

revert v2 changes

9168f5d

remove non-paginated methods

6ee22f5

remove old test stuff

10db888

stephenplusplus force-pushed the spp--23 branch from 969ec8d to 10db888 Compare March 12, 2018 20:20

stephenplusplus removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. status: blocked Resolving the issue is dependent on other work. labels Mar 12, 2018

alexander-fenster approved these changes Mar 12, 2018

View reviewed changes

kolodny approved these changes Mar 13, 2018

View reviewed changes

Use instanceof FamilyError check.

6bbc8bd

stephenplusplus added 5 commits March 13, 2018 10:29

rename options

e0eda51

Restore sinon.restore() call.

bbcbe25

Put back pre-request error handler

a7ccc12

prettier

4e5ca66

remove unnecessary resetHistory call

b236b42

kolodny merged commit 7abb15d into googleapis:master Mar 13, 2018

ghost removed the cla: yes This human has signed the Contributor License Agreement. label Mar 13, 2018

kolodny reviewed Mar 16, 2018

View reviewed changes

system-test/read-rows.js

);

assert.deepStrictEqual(requestedOptions, test.request_options);

setImmediate(function() {

This comment was marked as spam.

Sign in to view

kolodny mentioned this pull request Apr 17, 2018

Problem with boolean and decimal values #89

Closed

Convert to GAPIC #35

Convert to GAPIC #35

Conversation

stephenplusplus commented Jan 31, 2018 • edited Loading

Blockers

Breaking Changes

Todos

codecov bot commented Feb 15, 2018 • edited Loading

Codecov Report

stephenplusplus commented Feb 15, 2018

sduskis commented Feb 15, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

stephenplusplus commented Feb 27, 2018

kolodny commented Feb 28, 2018

stephenplusplus commented Feb 28, 2018

kolodny commented Mar 2, 2018

kolodny commented Mar 2, 2018

stephenplusplus commented Mar 2, 2018

kolodny commented Mar 2, 2018

stephenplusplus commented Mar 2, 2018

stephenplusplus commented Mar 2, 2018

kolodny commented Mar 2, 2018

stephenplusplus commented Mar 2, 2018

kolodny commented Mar 2, 2018

stephenplusplus commented Mar 2, 2018

stephenplusplus commented Mar 12, 2018

alexander-fenster left a comment

Choose a reason for hiding this comment

kolodny left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

sduskis commented Mar 13, 2018

stephenplusplus commented Mar 13, 2018

This comment was marked as spam.

stephenplusplus commented Jan 31, 2018 •

edited

Loading

codecov bot commented Feb 15, 2018 •

edited

Loading