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

adds retry option to event triggered functions #370

Closed
wants to merge 2 commits into from
Closed

Conversation

joehan
Copy link
Contributor

@joehan joehan commented Jan 3, 2019

Description

Adds retry option for event triggered functions, to match gcloud functions. https://cloud.google.com/functions/docs/reference/rpc/google.cloud.functions.v1#google.cloud.functions.v1.EventTrigger

Code sample

//adds an onCreate function for auth, with retry enabled
functions.runWith({retry:true})
.auth.user()
.onCreate(user => user);

Copy link
Contributor

@thechenky thechenky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unclear on how the retry option gets propagated to the CLI - does the CLI already pick up failurePolicy, or will we need to add that in?

@@ -168,6 +168,7 @@ export interface TriggerAnnotated {
httpsTrigger?: {};
eventTrigger?: {
eventType: string;
failurePolicy?: object;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious as to why failurePolicy is inside the eventTrigger and not outside like the other options like timeout and memory? was that because it does not apply to http functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is per the Google cloud docs - since it only applies to event-triggered functions, its a property of EventTrigger: https://cloud.google.com/functions/docs/reference/rest/v1/projects.locations.functions#EventTrigger

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, makes sense

@@ -303,7 +304,9 @@ export function makeCloudFunction<EventData>({
service,
},
});

if (opts.retry) {
trigger.eventTrigger.failurePolicy = { retry: {} };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this propagate the retry boolean instead of creating an empty object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope - strangely enough, google cloud expects an empty object of type Retry here: https://cloud.google.com/functions/docs/reference/rest/v1/projects.locations.functions#EventTrigger

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted, interesting

@joehan
Copy link
Contributor Author

joehan commented Jan 8, 2019

Hmm, just saw this old issue that was closed, with reasoning for NOT implementing this: #297 - thoughts?

@joehan
Copy link
Contributor Author

joehan commented Jan 8, 2019

To your comment about propagating - I believe the CLI already picks up the eventTrigger and sends the whole thing, so it shouldn't need any changes

@thechenky
Copy link
Contributor

Makes sense, thanks for explaining! Regarding whether we should implement this, I think @laurenzlong would have the context here. Lauren - have we set on the decision to not implement retries or have things changed since that time?

@thechenky
Copy link
Contributor

Chatted with Lauren offline - because this option will retry your function for 7 days, it's rather dangerous to expose as a native functionality in firebase-functions, as even the slightest error in your functions code can seriously rack up your bill. We will still continue exposing the ability to add a retry through the GCP console after a function has been deployed.

@joehan
Copy link
Contributor Author

joehan commented Jan 8, 2019

Closing since Firebase has chosen not to expose this potentially dangerous functionality - users who need this option can access it through GCF

@joehan joehan closed this Jan 8, 2019
@merlinnot
Copy link
Contributor

Is there any way it could be still exposed, but would require an explicit acknowledgement of a risk, either as a confirmation question in interactive shells or a flag for CI/CD systems?

@joehan
Copy link
Contributor Author

joehan commented Jan 8, 2019

@merlinnot : @laurenzlong - did this option come up in the discussion? Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants