pubsub: add ability to pass timeout to publish #2232

dennismartensson · 2017-04-18T18:15:39Z

Is based on #2042 to bring the ability to pass timeout to publish aswel.

Please have a look!

googlebot · 2017-04-18T18:15:42Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

stephenplusplus

Thank you so much for helping! Would you be able to add a test kind of like this one: https://github.com/GoogleCloudPlatform/google-cloud-node/pull/2042/files#diff-cc85884063a47cfc56746a81b8b768d5R414?

packages/pubsub/src/topic.js

@@ -456,6 +456,10 @@ Topic.prototype.publish = function(messages, options, callback) {
    method: 'publish',
  };

+  if (options && is.number(options.timeout)) {


dennismartensson · 2017-04-18T18:18:38Z

I signed it!

googlebot · 2017-04-18T18:18:40Z

CLAs look good, thanks!

dennismartensson · 2017-04-18T18:33:29Z

@stephenplusplus I added a test like the one ju linked to.

AUTHORS

@@ -11,3 +11,6 @@ Anand Suresh
 Brett Bergmann
 Jesse Friedman
 Zach Bjornson <[email protected]>
+
+Greta.io
+Dennis Mårtensson <[email protected]>


stephenplusplus · 2017-04-18T18:53:51Z

Looks awesome to me, thank you very much!

coveralls · 2017-04-18T18:57:22Z

Coverage remained the same at 100.0% when pulling c46ca54 on gretaio:pubsub-publish-timeout into 55d4d5f on GoogleCloudPlatform:master.

dennismartensson · 2017-04-18T19:45:03Z

@stephenplusplus How long before you think could be out so we can start mitigating the timeouts?

stephenplusplus · 2017-04-18T19:45:46Z

@dennismartensson I can merge and release as soon as @lukesneeringer can confirm the edit to the AUHTORS file is okay.

dennismartensson · 2017-04-18T19:58:27Z

@stephenplusplus Thank you!

dennismartensson · 2017-04-18T20:03:07Z

@stephenplusplus do you know what the default timeout value is?

packages/pubsub/src/topic.js

@@ -456,6 +456,10 @@ Topic.prototype.publish = function(messages, options, callback) {
    method: 'publish',
  };

+  if (is.number(options.timeout)) {
+    protoOpts.timeout = options.timeout;


stephenplusplus · 2017-04-18T20:44:00Z

do you know what the default timeout value is?

I'm not sure, this would be a default value that grpc sets.

Also, have you tested that this change solves the problem in #2230?

dennismartensson · 2017-04-18T20:46:27Z

Okey, I have not tested for #2231 don't get how that is related? (might be a miss type) but looks like it can be used to solve #2230 throwing to early timeouts.

stephenplusplus · 2017-04-18T20:50:33Z

Yes, it was a mis-type, sorry about that. I'm wondering if we should also default the timeout to a reasonable value, so users don't have to think about it.

dennismartensson · 2017-04-18T20:53:26Z

That might be a good idea but what should the default value be?

stephenplusplus · 2017-04-18T20:54:32Z

I'm not sure, what have you had luck with?

coveralls · 2017-04-18T21:01:36Z

Coverage remained the same at 100.0% when pulling 98d24aa on gretaio:pubsub-publish-timeout into 55d4d5f on GoogleCloudPlatform:master.

dennismartensson · 2017-04-18T21:08:15Z

I have only tried the default and 10 min just to make sure it went away.

dennismartensson · 2017-04-18T21:19:23Z

Found this:

// The default timeout in seconds for RPCs sent to this method. This can
      // be overridden in code. If no reply is received in the specified amount
      // of time, the request is aborted and a deadline-exceeded error status
      // is returned to the caller.
      //
      // The actual deadline used will be the minimum of the value specified
      // here and the value set by the application via the gRPC client API.
      // If either one is not set, then the other will be used.
      // If neither is set, then the request has no deadline.

Here

So I guess that means it could be set by the Google cloud pub/sub endpoint? With creates that error and maybe then default today is to follow the server?

stephenplusplus · 2017-04-19T11:57:58Z

Interesting. I think we should still override it. I can try to play with it later, but if you beat me to it, we want the lowest value that is consistently working. I would start at 10ms, go to 100ms, 1000ms, etc.

dennismartensson · 2017-04-20T15:00:44Z

I think it depends a bit on many messages are posted in one go. We we where posting just one message every time the feeling is we did not have as many of this error as we do now. So should we tune it for one message or many?

My quick tests points to 225 messages around 500byte per message 10sec looks okey, If I do 450 message per publish we seam to need a higher time. But do not feel like a well thought out strategy.

Have you had the time to run any tests?

stephenplusplus · 2017-04-20T17:05:12Z

This is good for now! I forgot we have a bit of a cheat sheet: https://github.com/GoogleCloudPlatform/google-cloud-node/blob/master/packages/pubsub/src/v1/publisher_client_config.json

I'll go through and see that we're setting these across the API in a follow-on PR. I'll release today so you can start using (at least) the change from this PR. Thank you again for doing this!

dennismartensson · 2017-04-21T05:26:21Z

Thank you @stephenplusplus for helping getting this out. The #2242 looks interesting!

pubsub: add ability to pass timeout to publish

3165709

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Apr 18, 2017

stephenplusplus suggested changes Apr 18, 2017

View reviewed changes

packages/pubsub/src/topic.js Outdated

@@ -456,6 +456,10 @@ Topic.prototype.publish = function(messages, options, callback) {

method: 'publish',

};

if (options && is.number(options.timeout)) {

This comment was marked as spam.

Sign in to view

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Apr 18, 2017

Dennis added 3 commits April 18, 2017 20:21

add name as contributer and remove a unnecessary check

66e13b8

fix lint error

13c08d9

tests

e2e8f4f

Dennis added 2 commits April 18, 2017 20:37

fix lint error in test

f96c40a

fix test

c46ca54

stephenplusplus reviewed Apr 18, 2017

View reviewed changes

dennismartensson mentioned this pull request Apr 18, 2017

Pub/Sub Deadline Exceeded #2230

Closed

stephenplusplus reviewed Apr 18, 2017

View reviewed changes

added JSDocs about the timeout on the options

98d24aa

stephenplusplus approved these changes Apr 18, 2017

View reviewed changes

stephenplusplus merged commit 19ab795 into googleapis:master Apr 20, 2017

stephenplusplus mentioned this pull request Apr 20, 2017

pubsub: borrow request config from GAX #2242

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pubsub: add ability to pass timeout to publish #2232

pubsub: add ability to pass timeout to publish #2232

dennismartensson commented Apr 18, 2017 •

edited by stephenplusplus

Loading

googlebot commented Apr 18, 2017

stephenplusplus left a comment

This comment was marked as spam.

dennismartensson commented Apr 18, 2017

googlebot commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

stephenplusplus commented Apr 18, 2017

coveralls commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

stephenplusplus commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

stephenplusplus commented Apr 18, 2017 •

edited

Loading

dennismartensson commented Apr 18, 2017

stephenplusplus commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

stephenplusplus commented Apr 18, 2017

coveralls commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

dennismartensson commented Apr 18, 2017 •

edited by stephenplusplus

Loading

stephenplusplus commented Apr 19, 2017

dennismartensson commented Apr 20, 2017

stephenplusplus commented Apr 20, 2017

dennismartensson commented Apr 21, 2017

pubsub: add ability to pass timeout to publish #2232

pubsub: add ability to pass timeout to publish #2232

Conversation

dennismartensson commented Apr 18, 2017 • edited by stephenplusplus Loading

googlebot commented Apr 18, 2017

stephenplusplus left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

dennismartensson commented Apr 18, 2017

googlebot commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

stephenplusplus commented Apr 18, 2017

coveralls commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

stephenplusplus commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

stephenplusplus commented Apr 18, 2017 • edited Loading

dennismartensson commented Apr 18, 2017

stephenplusplus commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

stephenplusplus commented Apr 18, 2017

coveralls commented Apr 18, 2017

dennismartensson commented Apr 18, 2017

dennismartensson commented Apr 18, 2017 • edited by stephenplusplus Loading

stephenplusplus commented Apr 19, 2017

dennismartensson commented Apr 20, 2017

stephenplusplus commented Apr 20, 2017

dennismartensson commented Apr 21, 2017

dennismartensson commented Apr 18, 2017 •

edited by stephenplusplus

Loading

stephenplusplus commented Apr 18, 2017 •

edited

Loading

dennismartensson commented Apr 18, 2017 •

edited by stephenplusplus

Loading