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 features to task queue functions #1423

Merged
merged 4 commits into from
Jun 26, 2023
Merged

Add features to task queue functions #1423

merged 4 commits into from
Jun 26, 2023

Conversation

blidd-google
Copy link
Contributor

@blidd-google blidd-google commented Jun 13, 2023

Since CF3 released support for Task Queue Functions at Google I/O 2022, users have made several feature requests. This PR augments the existing TaskContext interface to enable the following features:

  • Headers passed to TaskQueue HTTPS handlers are now available to the context/event.
  • Users can name their tasks by including an id to TaskOptions when enqueueing tasks.
  • Users can cancel their tasks by providing the task id to the new delete API.

See firebase/firebase-admin-node#2216 for the sister changes in the Admin SDK.

@blidd-google blidd-google requested review from inlined and taeold June 13, 2023 21:45
Copy link
Contributor

@taeold taeold left a comment

Choose a reason for hiding this comment

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

lgtm minor nits.

executionCount: number;

/**
* The schedule time of the task, as an RFC 3339 string in UTC time zone
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The schedule time of the task, as an RFC 3339 string in UTC time zone
* The schedule time of the task, as an RFC 3339 string in UTC time zone.


/**
* The HTTP response code from the previous retry.
* Populated via the X-CloudTasks-TaskPreviousResponse header
Copy link
Contributor

Choose a reason for hiding this comment

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

Would suggest putting the additional comments like this under @remark as done here https://github.com/firebase/firebase-functions/blob/master/src/common/providers/https.ts#L148

Or to put it as a single sentence. FYI everything under @remark tag won't show up in the reference docs.

const context: TaskContext = {
queueName: req.header("X-CloudTasks-QueueName"),
id: req.header("X-CloudTasks-TaskName"),
retryCount: Number(req.header("X-CloudTasks-TaskRetryCount")),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit* Can we make sure this won't below up if the request header isn't missing for some reason (e.g. when we invoke it in the emulator).

Would prefer having safe default behavior in case these headers are missing for whatever reason (maybe log a warning when that happens?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, the context object will just omit the missing headers if they are missing from the request object but still safely dispatch the handler with the context — what scenarios are you thinking about specifically?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh i thought this would blow up:

> Number(undefined)
NaN

Not sure if I like NaN but not as bad as I thought.

/**
* Raw request headers.
*/
headers?: Record<string, string>;
}

/**
* The request used to call a Task Queue function.
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhat out of scope here, but "Task Queue" does not need caps.


/**
* The name of the queue.
* Populated via the X-CloudTasks-QueueName header.
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc strings all look good!

Only question for me is whether "X-CloudTasks-QueueName" should be backticked as a literal. Or is the idea here that we are suggesting variables, where "QueueName" will actually be a unique name, and X maybe a number?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, per offline, it sounds like we should backtick it for code font as in https://firebase.google.com/docs/functions/callable-reference#request_format_headers and elsewhere.

const context: TaskContext = {
queueName: req.header("X-CloudTasks-QueueName"),
id: req.header("X-CloudTasks-TaskName"),
retryCount: Number(req.header("X-CloudTasks-TaskRetryCount")),
Copy link
Contributor

Choose a reason for hiding this comment

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

oh i thought this would blow up:

> Number(undefined)
NaN

Not sure if I like NaN but not as bad as I thought.

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.

4 participants