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

Add samples for RetryPolicy and filtering #1029

Closed
feywind opened this issue Jun 12, 2020 · 21 comments · Fixed by #1492 or #1700
Closed

Add samples for RetryPolicy and filtering #1029

feywind opened this issue Jun 12, 2020 · 21 comments · Fixed by #1492 or #1700
Assignees
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. priority: p3 Desirable enhancement or fix. May not be included in next release. samples Issues that are directly related to samples. type: docs Improvement to the documentation for an API.

Comments

@feywind
Copy link
Collaborator

feywind commented Jun 12, 2020

This is in response to these two issues:

#974
#976

It looks like the support is in for us to do this, but some samples could go a long way toward making it accessible.

@feywind feywind added priority: p2 Moderately-important priority. Fix may not be included in next release. type: docs Improvement to the documentation for an API. api: pubsub Issues related to the googleapis/nodejs-pubsub API. labels Jun 12, 2020
@feywind feywind self-assigned this Jun 12, 2020
@men232
Copy link

men232 commented Aug 14, 2020

no any progress?

@josedonizetti
Copy link
Contributor

@feywind any progress here? Tried to make it work with setMetadata in a number of ways, but keep getting errors.

INVALID_ARGUMENT: Invalid update_mask provided in the UpdateSubscriptionRequest: retry_policy is not a known Subscription field. Note that field paths must be of the form 'push_config' rather than 'pushConfig'.

@mgabeler-lee-6rs
Copy link
Contributor

@josedonizetti @sagimonza (from #974) I was able to get this working with the current v2.5.0 release.

Gist demoing it both at sub create time and via setMetadata: https://gist.github.com/mgabeler-lee-6rs/69a9b0e368d061b1935a7aa428e217b6

... I do get the error Jose mentions if I try to run it against the emulator however (have to comment out topic.setMetadata to get to the failure for the subscription however).

Interestingly, the emulator acts as if it supports setting the retryPolicy on create of a subscription, and will even return it back to you in getMetadata after that ... but it doesn't actually implement the retry policy.

@mgabeler-lee-6rs
Copy link
Contributor

It is quite frustrating that it seems you can't use almost any of the keys in setMetadata in the emulator. Makes it hard to have code using this library that can get tested in CI or used by developers on localhost 😞

@mahendraHegde
Copy link

does anyone know, how to set a filter for a subscription using this sdk? any docs??

@men232
Copy link

men232 commented Sep 16, 2020

does anyone know, how to set a filter for a subscription using this sdk? any docs??

  1. Make sure that you use latest version of this lib.
  2. Just pass filter property into subscription creating

For example

filter: [
attributes.platform = "dev",
attributes.label = "broker"
].join(' AND '),

@fiendfiend
Copy link

It is quite frustrating that it seems you can't use almost any of the keys in setMetadata in the emulator. Makes it hard to have code using this library that can get tested in CI or used by developers on localhost 😞

It seems the issues with setMetadata are not unique to the emulator. Trying to use it against real pubsub and it yields errors like this:

INVALID_ARGUMENT: Invalid update_mask provided in the UpdateTopicRequest: 'messageStoragePolicy' is not a known Topic field. Note that field paths must be of the form 'push_config' rather than 'pushConfig'.

I've attempted to use different variations, such as snake-casing, to coerce it:

message_storage_policy: {
  allowed_persistence_regions: ['europe-west2'],
},

This avoids the INVALID_ARGUMENT error, however no combination that I've tried seems to take any affect on the actual Topic in GCP.

UPDATE:

So it appears that somewhere along the line, the update mask value of 'messageStoragePolicy' is being converted to 'messageStoragepolicy' - where did that lower-case p come from???

image

when I debug into call-stream.sendMessageWithContext, I can see the serialised message still contains the valid upper-case P:

>>message.toString()
'
6
$projects/{OMITTED}/topics/TEST-62��
europe-west2��
�messageStoragePolicy'

It's not clear to me at what point this is becoming a lower-case p.

@ThomWright
Copy link

It is quite frustrating that it seems you can't use almost any of the keys in setMetadata in the emulator. Makes it hard to have code using this library that can get tested in CI or used by developers on localhost disappointed

Is there somewhere we can raise issues on the emulator?

@mgabeler-lee-6rs
Copy link
Contributor

Is there somewhere we can raise issues on the emulator?

On the theory that the emulator is part of the cloud sdk, I reported https://issuetracker.google.com/issues/165694293. It at least received an acknowledgement, though it's 50/50 to me whether that ack was actually from a human, or just a well written bot/template.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Dec 9, 2020
@meredithslota meredithslota removed 🚨 This issue needs some love. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Dec 11, 2020
@feywind
Copy link
Collaborator Author

feywind commented Jan 15, 2021

Taking a look at some old issues in here, a few comments:

  • @mgabeler-lee-6rs To be completely honest, there has been a lot of discussion about who owns the emulators and updates them with new features. I'm not sure there's been a resolution, but that seems like a reasonable place to file the request. Thanks for doing that. I think the intent is basically that it will cover the most commonly used features, but I agree that's a bit of a suboptimal experience.
  • @fiendfiend As far as the lower case p, I don't see that in the library itself, so I'm wondering if there's a case issue somewhere that you're passing settings to the library? If you're still having this issue and could paste the lines of code that call into the library to set the settings, that'd be helpful.
  • The added samples are definitely still in the queue to be done, so that hasn't been forgotten. We've been talking about some ways to better organize ongoing requests and questions, so I'm hoping that will help keep these issues from becoming stale.

@chrisatcloudwalk
Copy link

@feywind Any updates on the samples?

@C-Powers
Copy link

C-Powers commented Dec 16, 2021

Looking through some issues, it seems like it would look like this:

function main(
  topicName = 'YOUR_TOPIC_NAME',
  subscriptionName = 'YOUR_SUBSCRIPTION_NAME'
) {
  // [START pubsub_create_pull_subscription]
  /**
   * TODO(developer): Uncomment these variables before running the sample.
   */
  // const topicName = 'YOUR_TOPIC_NAME';
  // const subscriptionName = 'YOUR_SUBSCRIPTION_NAME';

  // Imports the Google Cloud client library
  const {PubSub} = require('@google-cloud/pubsub');

  // Creates a client; cache this for further use
  const pubSubClient = new PubSub();

  async function createSubscription() {
    // Creates a new subscription
    await pubSubClient.topic(topicName).createSubscription(subscriptionName, {
        filter: 'attributes.my-attr = my-filter'
    });
    console.log(`Subscription ${subscriptionName} created.`);
  }

  createSubscription().catch(console.error);
  // [END pubsub_create_pull_subscription]
}

@WesCossick
Copy link

@feywind & @anguillanneuf Just a heads up, the just-merged PR that closed this issue did not fully address the issue. The PR only added samples for subscriptions with filtering—not for subscriptions with a retry policy.

@anguillanneuf
Copy link
Contributor

@WesCossick You are right. The sample is going to be published here: https://cloud.google.com/pubsub/docs/filtering#creating_subscriptions_with_filters. It doesn't have anything to do with RetryPolicy. It's unrelated.

@feywind
Copy link
Collaborator Author

feywind commented Feb 23, 2022

@WesCossick I thought I'd commented here, sorry - at the moment we don't have a tag for the docs for a subscription retry policy sample, but I'm okay leaving this open to track that.

@feywind feywind reopened this Feb 23, 2022
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Feb 23, 2022
@feywind
Copy link
Collaborator Author

feywind commented Feb 23, 2022

@anguillanneuf pointed me at this:

https://cloud.google.com/pubsub/docs/admin#pubsub_update_push_configuration-nodejs

which is also this:

https://github.com/googleapis/nodejs-pubsub/blob/main/samples/modifyPushConfig.js

You should be able to do a similar thing by calling setMetadata with your retry policy instead of modifyPushConfig. I can add a "non-canonical" sample for that if it would be helpful. (And/or try to get a sample tag made, but that would trigger a bunch of other languages needing those samples.)

@anguillanneuf
Copy link
Contributor

@feywind is right. @WesCossick, are you able to modify that sample to update retry_policy? We don't have plans to add a sample for every update operation. Hope you understand.

@WesCossick
Copy link

I'd have to dig into the code where we use retry policies and get back on that. I just wanted to quickly point out that this issue was created specifically to combine #974 and #976 into a single issue, and then those two issues were closed, but the PR ultimately only addressed #976.

@anguillanneuf
Copy link
Contributor

@feywind (Maybe you already have, but) can we verify if this is true? #974 (comment)

We don't need to publish a docs sample but we can include some code snippets as part of the library reference. For examples:

@feywind feywind added the priority: p3 Desirable enhancement or fix. May not be included in next release. label Aug 15, 2022
@greensopinion
Copy link

This is what I'm using to create a subscription with metadata such as retry policy in NodeJS:

https://gist.github.com/greensopinion/ce5ebc6a8d76d2978634a089ead082b6

@feywind
Copy link
Collaborator Author

feywind commented Mar 7, 2023

@greensopinion Thanks for the gist. I think we're going to add a sample for this after all. There was some debate about whether the sample browser site was for any sample or only for ones in the doc pages.

feywind pushed a commit to feywind/nodejs-pubsub that referenced this issue Nov 12, 2024
googleapis#1029)

* fix: do not cancel stream after server returned ok or cancelled status

* remove comment

* lint

* lint
feywind pushed a commit to feywind/nodejs-pubsub that referenced this issue Nov 12, 2024
🤖 I have created a release *beep* *boop*
---


### [3.9.4](googleapis/nodejs-bigtable@v3.9.3...v3.9.4) (2022-03-16)


### Bug Fixes

* do not cancel stream after server returned ok or cancelled status ([googleapis#1029](googleapis/nodejs-bigtable#1029)) ([33754a2](googleapis/nodejs-bigtable@33754a2))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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. priority: p3 Desirable enhancement or fix. May not be included in next release. samples Issues that are directly related to samples. type: docs Improvement to the documentation for an API.
Projects
None yet