Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pub/Sub retry settings: needs common api between data & admin ops, better docs. #35

Closed
stephenplusplus opened this issue Jan 9, 2018 · 9 comments
Assignees
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. status: blocked Resolving the issue is dependent on other work. triaged for GA type: question Request for information or clarification. Not an issue.

Comments

@stephenplusplus
Copy link
Contributor

Copied from original issue: googleapis/google-cloud-node#2788

@kir-titievsky
January 9, 2018 4:42 PM

This is a bug to track progress on an email discussion about setting custom retry settings.

Previous suggestion:
In src/v1/publisher_client.js the gapicConfig contains the contents of the config file you mentioned, and it is passed down to gaxGrpc:

var defaults = gaxGrpc.constructSettings(

var defaults = gaxGrpc.constructSettings(
  'google.pubsub.v1.Publisher',
  gapicConfig,
  opts.clientConfig,
  {'x-goog-api-client': clientHeader.join(' ')}
);

Here the third parameter, opts.clientConfig, gives configOverrides to gaxGrpc.constructSettings, and, given the name, I guess it might be able to override options.
opts.clientConfig comes from PublisherClient constructor parameter. So I guess you might try settings opts.clientConfig to override any default configuration.

If it does not work for you, please let me know - better with code example - and I'll create an issue for pubsub package.

@theacodes
Copy link

@kir-titievsky do you think this is release blocking? Does this affect the interface or is it something that can be changed post v1.0.0 without necessitating a major version bump?

@kir-titievsky
Copy link

I think this is release blocking, unless we are convinced that offering a consistent retry settings API across all operations will not require client library API changes.

@stephenplusplus
Copy link
Contributor Author

stephenplusplus commented Feb 12, 2018

Anything that is passed to the PubSub constructor gets passed to PublisherClient. Has the issue reporter tried the suggestion from the end of the opening post in this thread? (Seems like we are only seeing a piece of the conversation)

@stephenplusplus
Copy link
Contributor Author

Should we close this one?

@stephenplusplus stephenplusplus added status: blocked Resolving the issue is dependent on other work. type: question Request for information or clarification. Not an issue. labels Apr 2, 2018
@lazharichir
Copy link

The current release 0.18.0 seems to offer this possibility of customizing retry settings from the Publisher, yet, it doesn't work for me.

pubsub.topic(topicName).publisher({ gaxOpts: { retry: null } }).publish(message);

I receive that message every seconds and it fails every single time (pushed to an HTTP-triggered Google Cloud Functions).

No documentation released yet but I dug in the NPM module's code. Any specific format to follow that I got wrong for the gaxOpts.retry ?

stephenplusplus pushed a commit that referenced this issue Jun 11, 2018
## Version **0.16.0** of [google-proto-files](https://github.com/googleapis/nodejs-proto-files) was just published.

<table>
  <tr>
    <th align=left>
      Dependency
    </th>
    <td>
      <code>google-proto-files</code>
    </td>
  </tr>
  <tr>
      <th align=left>
       Current Version
      </th>
      <td>
        0.15.1
      </td>
    </tr>
  <tr>
    <th align=left>
      Type
    </th>
    <td>
      dependency
    </td>
  </tr>
</table>

The version **0.16.0** is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

It might be worth looking into these changes and trying to get this project onto the latest version of google-proto-files.

If you have a solid test suite and good coverage, a passing build is a strong indicator that you can take advantage of these changes directly by merging the proposed change into your project. If the build fails or you don’t have such unconditional trust in your tests, this branch is a great starting point for you to work on the update.


---


<details>
<summary>Release Notes</summary>
<strong>v0.16.0</strong>

<h2>Features</h2>
<ul>
<li>(<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="316092585" data-permission-text="Issue title is private" data-url="googleapis/nodejs-proto-files#35" href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/pull/35">#35</a>): Allow passing <code>load()</code> options to protobuf.js. (Thanks, <a class="user-mention" data-hovercard-user-id="583593" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/nmccready">@nmccready</a>!)</li>
</ul>
</details>

<details>
<summary>Commits</summary>
<p>The new version differs by 19 commits.</p>
<ul>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/b4d00bd0b7e6c2d4e0be1395739ded62bd9083e3"><code>b4d00bd</code></a> <code>0.16.0 (#44)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/47488981af49c1102737e60cad560f9609ac29d2"><code>4748898</code></a> <code>feat: allow load options (#35)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/91a34aa6ec2fb31932a03f4ed7756210146ea163"><code>91a34aa</code></a> <code>Merge pull request #42 from googleapis/greenkeeper/nyc-12.0.1</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/fca6f7540c45b1ec3916602e1189cef289429908"><code>fca6f75</code></a> <code>Update package.json</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/f0473ada67034a0a294d3f15dd1aa968acb5750a"><code>f0473ad</code></a> <code>chore(package): update nyc to version 12.0.1</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/057563111be9bf9e97e02f893fbed08a9b465db2"><code>0575631</code></a> <code>chore: lock files maintenance (#41)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/5f38e7f77b6c14a5bbc256acb598fe10f4dbb693"><code>5f38e7f</code></a> <code>chore: lock files maintenance (#38)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/d077e0b86bf56135301a5240a15c4cbbabf2eb47"><code>d077e0b</code></a> <code>chore: test on node10 (#37)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/7855ccaca6a1f323719ef2c5a6c77ddd06953cb6"><code>7855cca</code></a> <code>chore: lock files maintenance (#36)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/e5548189863303d1973d54c5dc23d5eb599fbdaa"><code>e554818</code></a> <code>Merge pull request #32 from googleapis/greenkeeper/sinon-5.0.0</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/0ecbb5e631d5918b8e1cf09e6ab1b6ed6d3baa64"><code>0ecbb5e</code></a> <code>Update package locks</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/838e2d1d84db3e15a0a5a73242ac7399709d6a44"><code>838e2d1</code></a> <code>chore(package): update sinon to version 5.0.0</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/fb18058f58c906ff9e90be80d4f312dbfb9ff68f"><code>fb18058</code></a> <code>chore: workaround for repo-tools EPERM (#33)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/cf5968e2974b0cecbb6a807ffde696ec37dd6fe6"><code>cf5968e</code></a> <code>chore: setup nighty build in CircleCI (#31)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/c4c973a41ee5810052fb0159ae2f83c18e6f866f"><code>c4c973a</code></a> <code>chore(package): update proxyquire to version 2.0.0 (#30)</code></li>
</ul>
<p>There are 19 commits in total.</p>
<p>See the <a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/compare/89183d7d25f9c7e3b9097da820d9d84bd803a92f...b4d00bd0b7e6c2d4e0be1395739ded62bd9083e3">full diff</a></p>
</details>

<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴
stephenplusplus pushed a commit to stephenplusplus/nodejs-pubsub that referenced this issue Aug 31, 2018
@callmehiphop callmehiphop self-assigned this Nov 12, 2018
@jkwlui
Copy link
Member

jkwlui commented Dec 4, 2018

is this still an issue with the latest versions? @callmehiphop

@callmehiphop
Copy link
Contributor

@kinwa91 I'm actually not 100% what the issue is describing, haha..

In regards to gapics, I can't tell if gax actually uses/caches any call options passed in via the client constructor, but as far as I can tell, it does not.

If this is about the hand-written Publisher client, it should be resolved.

@callmehiphop
Copy link
Contributor

I think as far as providing publishing call options, this has been solved for a while. We could probably improve the client with the steps outlined in googleapis/google-cloud-node#2895, but I'll let @kir-titievsky make the call if that should hold up GA or not. Going to close this issue, but please feel free to re-open if you disagree!

@sduskis
Copy link

sduskis commented May 15, 2019

We should do a thorough review of the API before we go GA. We ought to do that sooner rather than later. I'll bring it up with Kir and crew.

@google-cloud-label-sync google-cloud-label-sync bot added the api: pubsub Issues related to the googleapis/nodejs-pubsub API. label Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. status: blocked Resolving the issue is dependent on other work. triaged for GA type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

No branches or pull requests

7 participants