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 v2 Schedule Triggers #1177

Merged
merged 16 commits into from
Aug 25, 2022
Merged

Add v2 Schedule Triggers #1177

merged 16 commits into from
Aug 25, 2022

Conversation

colerogers
Copy link
Contributor

@colerogers colerogers commented Jul 21, 2022

Adds schedule triggers to the v2 namespace.

  • Replaces pub/sub with an http underlying function
  • Updated syntax to match with the other v2 triggers

This change does not add the invoker property to the container contract.

export const sch = v2.scheduler.onSchedule("* * * * *", () => {});

export const sch = v2.scheduler.onSchedule(
  {
    schedule: "* * * * *",
    timeZone: "utc",
  },
  () => {}
);

@Berlioz
Copy link
Contributor

Berlioz commented Jul 21, 2022

Note to self: some of the types in this trigger are also going to need the Field/Expression treatment.

@colerogers
Copy link
Contributor Author

@Berlioz I can update this PR to take the work off your plate, what example should I be looking at?

@colerogers colerogers marked this pull request as ready for review July 22, 2022 17:16
src/v2/providers/schedule.ts Outdated Show resolved Hide resolved
Comment on lines 36 to 38
maxRetryDuration?: string;
minBackoffDuration?: string;
maxBackoffDuration?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not the best time to have this discussion, but I'm surprised that this is *Duration instead of *Seconds. That's what we did for Task Queue Functions (even though the underlying Tasks API uses *Duration).

On one hand, I'd like consistency across v2. On the other hand, this breaks consistency w/ v1 scheduled function! What do you think we should do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm that's an interesting point. A counter argument is that the user could have a string value that's not in seconds. The API uses 1h as an example in this case. IDK if we should name the interface something that could limit the customers choices. But on the other hand, we try to have an opinionated SDK. 🤷‍♂️

Also, I'm not sure what you mean by breaking consistency w/ v1 scheduled functions since they use the Duration naming too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Meant that *Seconds breaks consistency w/ Task Queue API. Lots of parameters here look like TQ API's parameter - I would find it weird to see TQ and Scheduled function use slightly different parameters for configuration.

Copy link
Contributor Author

@colerogers colerogers Aug 3, 2022

Choose a reason for hiding this comment

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

After internal discussion, I'll change from *Duration to *Seconds for the v2 api.

If we're going to include this in the v4 SDK, should we make the change in the v1 syntax to *Seconds too?

Copy link
Contributor

@taeold taeold Aug 12, 2022

Choose a reason for hiding this comment

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

I'd vote for keeping v1 the same. We should fix the container contract representation to convert duration to seconds though.

src/v2/providers/schedule.ts Outdated Show resolved Hide resolved
src/v2/providers/schedule.ts Outdated Show resolved Hide resolved
src/v2/providers/schedule.ts Outdated Show resolved Hide resolved
}

res.setHeader('Content-Type', 'application/json');
res.status(200).send();
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we always respond w/ 200? Doesn't this mean that we won't retry execution w/ errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current behavior for v1 is that a function error will not retry the execution. A retry will occur when the scheduler job fails. I opted for consistency and just return a 200. Let me know if you think we should change this up for v2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm following footsteps of v1 scheduled function seems reasonable, though I would guess having scheduled functions automatically retry on transient errors to be pretty useful. This is something we explicitly set for event functions via failurePolicy and I can see it being applied here too.

But if you think that is a feature that doesn't need to be implemented v2 scheduled function, that sounds fine with me. Maybe we'll get a FR for it one day, and we can implement it then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SG, updated to send a 500 if we catch an error.

src/v2/providers/schedule.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/v2/providers/schedule.ts Outdated Show resolved Hide resolved
@taeold
Copy link
Contributor

taeold commented Aug 25, 2022

Force merging - @inlined's comments have all been addressed.

@taeold taeold merged commit eed7d59 into master Aug 25, 2022
@taeold taeold deleted the colerogers.schedule branch August 25, 2022 16:52
@kbi-daniel
Copy link

The new V2 onSchedule cloud function does not seem to have a way to set timeoutSeconds or memory options.

For instance, my V1 scheduled cloud function currently sets both using runWith():
exports.function = functions.runWith({timeoutSeconds: 300, memory: '512MB'}).pubsub.schedule('0 22 * * 0-6').onRun(() => {...});

However, the currently listed ScheduleOptions listed in the V2 documentation does not list either of these properties:
https://firebase.google.com/docs/functions/beta/reference/firebase-functions.scheduler.scheduleoptions

Are these properties going to be added to ScheduleOptions?

@taeold
Copy link
Contributor

taeold commented Feb 3, 2023

Hey @kbi-daniel! The ScheduleOptions extends the base GlobalOptions, so options like timeout and memory should be available. Eg.

exports.v2schedule = onSchedule(
    {
        // ScheduleOptions
        schedule: "every 2 minutes",
        maxBackoffSeconds: 20,
        // GlobalOptions
        memory: "1GiB",
        timeoutSeconds: 50,
    }, (event) => {
    console.log(event);
});

@kbi-daniel
Copy link

Thanks @taeold. Worked as expected!

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

Successfully merging this pull request may close these issues.

5 participants