Add smart retries to table.mutate

[kolodny]

• Tests and linter pass
• Code coverage does not decrease (if any source code was changed)

A couple of things to note about this PR:

• There's a bug where faketimers don't play nicely streams. I filed a bug on process-nextick-args which causes an issue of readable-streams, so that's the deal with the patch files. Node v4 has a nested node_modules structure while >= v6 is flat.
• patch-package doesn't handle possible missing packages so that's why the git commands are run manually

[codecov-io]

Codecov Report

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

@@          Coverage Diff          @@
##           master    #17   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines         792    811   +19     
=====================================
+ Hits          792    811   +19

Impacted Files	Coverage Δ
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 b85679b...e4c03fa. Read the comment docs.

[kolodny]

I can get coverage for unit tests back to 100% with the following patch, although I'm not sure it's the right thing to do

diff --git a/test/table.js b/test/table.js
index f348120..9173788 100644
--- a/test/table.js
+++ b/test/table.js
@@ -999,8 +999,10 @@ describe('Bigtable/Table', function() {
     describe('error', function() {
       describe('API errors', function() {
         var error = new Error('err');
+        var setTimeoutStub;

         beforeEach(function() {
+          setTimeoutStub = sinon.stub(global, 'setTimeout').callsFake(setImmediate);
           table.requestStream = function() {
             var stream = new Stream({
               objectMode: true,
@@ -1015,6 +1017,10 @@ describe('Bigtable/Table', function() {
           };
         });

+        afterEach(function() {
+          setTimeoutStub.restore();
+        });
+
         it('should return the error to the callback', function(done) {
           table.maxRetries = 0;
           table.mutate(entries, function(err) {
@@ -1022,6 +1028,17 @@ describe('Bigtable/Table', function() {
             done();
           });
         });
+
+        it('attempts to retry the mutation', function(done) {
+          table.maxRetries = 1;
+          table.mutate(entries, function(err) {
+            assert.strictEqual(err, error);
+            assert.strictEqual(setTimeoutStub.callCount, 1);
+            assert(setTimeoutStub.getCall(0).args[1] > 2000);
+            assert(setTimeoutStub.getCall(0).args[1] < 3000);
+            done();
+          });
+        });
       });

       describe('mutation errors', function() {

[stephenplusplus]

This PR introduces an "integration" test command, but we already have system tests that are supposed to be the same thing: "Does our code work when integrated with the system?" (Using the name "system tests" was a decision that involved more languages than just us). I also see a reference to "acceptance" testing. I think we should keep everything under one roof, "system tests", since that's our pattern across all of the API libraries. It should only be one command for a contributor/maintainer to determine if their changes introduce any issues with the upstream API, and that's historically been "npm run system-test".

Regarding the patching, I would prefer to wait for calvinmetcalf/process-nextick-args#10 to be merged and released.

[kolodny]

Regarding patching, after calvinmetcalf/process-nextick-args#10 is merged (it already was), nodejs/readable-stream#317 needs to be merged, publish a release (note this module is basically part of node core), and then wait for through2 to publish a release with that updated readable-stream dependency. I figured this was a good enough workaround in the meantime

[stephenplusplus]

I see, thanks for the rundown. Yeah, I suppose the patching will have to do for now.

[stephenplusplus]

Thanks for splitting that out. I believe there might be some work for the code coverage still.

https://codecov.io/gh/googleapis/nodejs-bigtable/pull/17/src/src/table.js?before=src/table.js#L942

[kolodny]

I can add the following test to the unit tests to get it back to 100%. Is that something you'd be ok with?

    describe('retries', function() {
      var fakeStatuses = [
        [
          {
            status: {
              code: 0,
            },
          },
          {
            status: {
              code: 4,
            },
          },
        ],
        [
          {
            status: {
              code: 0,
            },
          },

        ]
      ];

      beforeEach(function() {
        FakeGrpcService.decorateStatus_ = function(status) {
          return {};
        };
        table.requestStream = function() {
          var stream = new Stream({
            objectMode: true,
          });

          setImmediate(function() {
            stream.emit('request');
            stream.end({entries: fakeStatuses.shift()});
          });

          return stream;
        };
      });

      it('should succeed after a retry', function(done) {
        table.maxRetries = 1;
        table.mutate(entries, done);
      });
    });
  });

[stephenplusplus]

That test looks good to me. I also think we should have another unit test to confirm only the correct subset of entries from the original request is sent when a retry is made, effectively testing the new pendinEntryIndices, entryToIndex holders.

[kolodny]

Done, coverage appears to be back to 100%

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[stephenplusplus]

I made some small changes, just to conventionalize the code (stream error handling first, all caps file-wide const name, etc). Tests still pass, but please make sure these changes look harmless to you as well.

[kolodny]

Look good to me!