Intent to implement: retry option for event triggered functions

[merlinnot]

Overview

Based on a discussion in #370 I'd like to propose an improved version of the feature that should cover the concerns raised in the original implementation. I'm willing to work on adding this feature.

Design proposal

Definitions

Let new function with retries be a Cloud Function which has the option for reties set and it's previous deployment did not have reties enabled.

Changes in firebase-functions

Extend an interface of RuntimeOptions of a FunctionBuilder in the following way:

interface FailurePolicy {
  retry: {};
}

interface RuntimeOptions {
  failurePolicy: FailurePolicy;
  memory?: '128MB' | '256MB' | '512MB' | '1GB' | '2GB';
  timeoutSeconds?: number;
}

I'd most likely dive a little deeper to make sure that this option is not present for https triggers.

Some might wonder why a retry option is an object. I followed the interface of the Cloud Functions API which specifies it this way, most probably to support customization of this policy in the future in a non-breaking way. That's why I decide to follow this approach here.

Changes in firebase-tools

Add a confirmation step to the deployment process for new function with retries with the following behavior:

• Interactive shells: display a confirmation prompt with the following text: "The following functions will be retried in case of a failure: fnA, fnB, fnC [a list of all functions with retries, not only new function with retries]. Please make sure all of these functions are idempotent, see https://cloud.google.com/blog/products/serverless/cloud-functions-pro-tips-building-idempotent-functions to learn more. Retried execution is charged as any other execution. Would you like to proceed?"
• In non interactive shells: display an error message (printed to standard error stream) with the following text: "The following functions have a retry policy: fnA, fnB, fnC. Please make sure all of these functions are idempotent, see https://cloud.google.com/blog/products/serverless/cloud-functions-pro-tips-building-idempotent-functions to learn more. Retried execution is charged as any other execution. Supply a --force option to suppress this error." and exit the process with code 1.
• In non interactive shells with --force option: proceed with the deployment.

[google-oss-bot]

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

[kevinajian]

Hey @merlinnot,

Thanks for the feedback. I'll bring this up for discussion again with your new considerations.

For now, you can still enable retries through the Cloud Console.

[joehan]

Hey @merlinnot, we discussed your proposal and we think its a pretty reasonable way to introduce retries, and we'd like to move forward. Some quick design notes - let me know if they sound good to you:

• theres no need to include the warning prompt when updating a function that already has retires enabled.
• Could you include a link to https://cloud.google.com/blog/products/serverless/cloud-functions-pro-tips-building-idempotent-functions in the warning? It should help users safely retry functions.

Since this is a public API, we need to run your proposal past an internal council before we can accept any PRs. I'm going to submit it today, so it should be 1-2 weeks before we get it approved. I'd hold off on starting implementation until we have approval - I think this proposal will likely make it through, but I'd hate to waste your hard work if it doesn't.

Also, thanks for offering to implement this - you rock!

[merlinnot]

Great, I'll adjust the proposal accordingly as soon as I find some time (within 24hrs).

[merlinnot]

Ok, I updated the proposal. Let me know what you think :)

[joehan]

Hey @merlinnot, lI've submitted the proposal to the API review council - its scheduled to be reviewed on 4/8/2019. I'll post here again with the results, but I think it looks good .

[merlinnot]

@joehan, do we have any update on this?

[joehan]

Hey @merlinnot, super sorry for the delay on this. Just checked in with the approvers - we made some minor changes to the proposal, and they are at LGTM pending one question about how to handle the following situation: a user has set a retry policy manually on a function, and then they redeploy after this change goes in. We want to make sure that they do not accidentally unset their retry policy without realizing it. I proposed adding a warning for unsetting any retry policies to mitigate this, so they will (hopefully) officially approve this early next week.

[merlinnot]

Great, thanks!

[joehan]

Hey @merlinnot, good news! Your proposal was accepted! The council had a few implementation notes:

• We are a little worried about unsetting existing retry policies without user noticing. Therefore, whenever the CLI would delete a retry policy, we should show the following warning: 'You are about to disable retries for functionA. Would you like to proceed? Y/n'

• The interface for Runtime options should instead be:

interface RuntimeOptions {
  retry?: FailurePolicy | boolean ;
  memory?: '128MB' | '256MB' | '512MB' | '1GB' | '2GB';
  timeoutSeconds?: number;
}

If the user provides the boolean true, this will be equivalent to providing an empty failure policy. The idea behind this change is to support a simple interface for now while still being ready to accept more complex failure policies if they are added.

• The reviewers asked for some slight language changes:

Interactive shells: display a confirmation prompt with the following text: "The following functions will be retried in case of a failure: fnA, fnB, fnC [a list of all functions with retries, not only new function with retries]. Retried executions are billed as any other execution, and function are retried repeatedly until they either successfully execute or the maximum retry period has elapsed, which can be multiple days. For safety, you might want to ensure that your functions are idempotent; see https://firebase.google.com/docs/functions/retries to learn more. Would you like to proceed?"

In non interactive shells: display an error message (printed to standard error stream) with the following text: "The following functions have a retry policy: fnA, fnB, fnC [a list of all functions with retries, not only new function with retries]. Please make sure all of these functions are safe to retry, see https://firebase.google.com/docs/functions/retries to learn more. Retried execution is charged as any other execution. Supply a --force option to suppress this error." and exit the process with code 1.

In non interactive shells with --force option: log the following message and proceed with the deployment.
"The following functions have a retry policy: fnA, fnB, fnC. Retried executions are billed as any other execution, and functions are retried repeatedly until they either successfully execute or the maximum retry period has elapsed, which can be multiple days.”

[merlinnot]

Great news! I'm on holiday next week, will pick up the implementation soon after.

Thanks for helping me out with getting it through the council 👍

[joehan]

Sounds great! And my pleasure, we always love community contributions :)

[thechenky]

Hi @merlinnot just checking in on this - are you still planning to implement this feature?

[merlinnot]

Yes. I started to go through the source code of firebase-tools and firebase-functions to get a better understanding of what needs to be done, which resulted in this PR: #450 and some opened issues.

And then I go caught up in other priorities. I have two things on my TODO list right now: this issue and firebase/firebase-tools#1372. I'll do it as soon as I can, I really need both of these, but it won't be this week for sure.

[thechenky]

Sounds good, thanks for the update!

[jullui]

@merlinnot is there any update regarding this feature?

[merlinnot]

@jullui Take a look at #482 (comment)