pubsub: refactor subscription connection #2627

callmehiphop · 2017-09-25T14:14:42Z

stephenplusplus

Small stuff and questions.

packages/pubsub/src/connection-pool.js

@@ -32,6 +32,9 @@ var uuid = require('uuid');
 var PKG = require('../package.json');
 var v1 = require('./v1');

+var CHANNEL_READY = 'channel.ready';


packages/pubsub/src/connection-pool.js

+      connection.pause();
+    }
+
+    self.once(CHANNEL_ERROR, function(err) {


packages/pubsub/src/connection-pool.js

+    }
+
+    var elapsedTimeWithoutConnection = 0;
+    var deadline, timeout;


packages/pubsub/src/connection-pool.js

+ */
+ConnectionPool.prototype.waitForReady = function() {
+  var self = this;
+  var READY = 2;


packages/pubsub/src/connection-pool.js

+/**
+ *
+ */
+ConnectionPool.prototype.watchForReady = function() {


packages/pubsub/src/connection-pool.js

+
+  this.on('newListener', function(eventName) {
+    if (eventName == CHANNEL_READY && !self.waiting) {
+      clearImmediate(self.waitHandle);


packages/pubsub/src/connection-pool.js

+  this.isOpen = false;
+  this.waiting = false;
+
+  this.removeAllListeners('newListener')


packages/pubsub/src/connection-pool.js

+    });
+
+    self.once(CHANNEL_READY, function() {
+      connection.isConnected = true;


coveralls · 2017-09-27T20:40:50Z

Coverage remained the same at 100.0% when pulling 728575b on callmehiphop:dg--pubsub-stream-refactor into 42eef91 on GoogleCloudPlatform:master.

callmehiphop · 2017-09-27T20:41:16Z

@stephenplusplus I think this is ready for another look through.

packages/pubsub/package.json

@@ -59,6 +59,7 @@
    "google-auto-auth": "^0.7.1",
    "google-gax": "^0.13.0",
    "google-proto-files": "^0.12.0",
+    "grpc": "^1.6.0",


packages/pubsub/test/connection-pool.js

+          pool.createConnection();
+          pool.emit(channelErrorEvent);
+
+          assert.strictEqual(fakeConnection.canceled, true);


packages/pubsub/test/connection-pool.js

+      pool.getClient = function(callback) {
+        callback(fakeError);
+
+        assert.strictEqual(pool.isGettingChannelState, false);


packages/pubsub/test/connection-pool.js

+      getChannel: function() {
+        return {
+          getConnectivityState: function(shouldConnect) {
+            assert.strictEqual(shouldConnect, false);


packages/pubsub/src/connection-pool.js

+      elapsedTimeWithoutConnection = now - self.noConnectionsTime;
+    }
+
+    deadline = now + (300000 - elapsedTimeWithoutConnection);


packages/pubsub/src/subscription.js

@@ -520,6 +523,9 @@ Subscription.prototype.exists = function(callback) {
 Subscription.prototype.flushQueues_ = function() {
  var self = this;

+  clearTimeout(this.flushTimeoutHandle_);


coveralls · 2017-09-28T19:13:50Z

Coverage remained the same at 100.0% when pulling 8f88c29 on callmehiphop:dg--pubsub-stream-refactor into 42eef91 on GoogleCloudPlatform:master.

callmehiphop · 2017-10-01T15:49:11Z

@stephenplusplus this should be good to go btw!

ApeNox · 2017-10-02T08:06:29Z

Could this be merged ? seems it fixes the issue with acked messages.

pubsub: refactor subscription connection

3013edb

callmehiphop added api: pubsub Issues related to the Pub/Sub API. status: do not merge labels Sep 25, 2017

callmehiphop requested a review from lukesneeringer as a code owner September 25, 2017 14:14

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 25, 2017

refactored client.waitForReady usage

ce1a803

callmehiphop removed the windows label Sep 25, 2017

stephenplusplus reviewed Sep 27, 2017

View reviewed changes

callmehiphop added 3 commits September 27, 2017 16:29

lock down new grpc version

856f57d

refactor per PR feedback

8c1074b

unit tests

728575b

callmehiphop removed the status: do not merge label Sep 27, 2017

stephenplusplus reviewed Sep 28, 2017

View reviewed changes

callmehiphop added 2 commits September 28, 2017 14:40

bump grpc version in common-grpc

dc1a372

minor adjustments per pr feedback

8f88c29

stephenplusplus merged commit 85ac80a into googleapis:master Oct 2, 2017

lostpebble mentioned this pull request Oct 27, 2017

Latest pubsub version breaks dependencies with grpc (doesn't use common-grpc package) #2699

Closed

callmehiphop mentioned this pull request Nov 8, 2017

Error: Endpoint read failed at /user_code/node_modules/@google-cloud/logging/node_modules/grpc/src/node/src/client.js:569:15 #2438

Closed

pubsub: refactor subscription connection #2627

pubsub: refactor subscription connection #2627

Conversation

callmehiphop commented Sep 25, 2017

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

coveralls commented Sep 27, 2017

callmehiphop commented Sep 27, 2017

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.

coveralls commented Sep 28, 2017

callmehiphop commented Oct 1, 2017

ApeNox commented Oct 2, 2017