-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: sync scheduled function timeout with attempt deadline #9544
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
Conversation
40e8e7f to
6a9d74a
Compare
Summary of ChangesHello @taeold, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the management of scheduled Firebase functions by removing the explicit Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request simplifies the configuration for scheduled functions by removing the user-facing attemptDeadlineSeconds and instead synchronizing the Cloud Scheduler's attempt deadline with the function's timeoutSeconds. The implementation correctly caps the deadline at 30 minutes and floors it at 180 seconds to prevent premature retries, which is a sensible default. The changes are consistently applied across interfaces, validation logic, and tests. Overall, this is a solid improvement that reduces complexity for users.
### Description Automatically synchronizes Cloud Scheduler's attemptDeadline with the function's timeoutSeconds for v2 scheduled functions, capped at 1800 seconds. - Removes attemptDeadlineSeconds as a user-configurable field in the manifest. - Adds a warning when timeoutSeconds exceeds 1800 seconds for scheduled functions. - Refactors validation logic for scheduled timeouts. ### Scenarios Tested - Unit tests for cloudscheduler.ts and validate.ts. - Verified that attemptDeadline is correctly set and capped. - Verified warning message is logged when timeout exceeds 1800s.
6a9d74a to
e06fb91
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly synchronizes the Cloud Scheduler attemptDeadline with the function's timeoutSeconds, including the capping and flooring logic. The removal of the user-facing attemptDeadlineSeconds configuration is a good simplification and has been applied consistently. I've added a few suggestions to improve maintainability by removing magic numbers and duplicated constants, and to clarify a comment in a test. Overall, this is a solid improvement.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request synchronizes the Cloud Scheduler's attemptDeadline with the function's timeoutSeconds for v2 scheduled functions, which is a great simplification from the previous approach. The implementation looks solid, and the new tests cover the capping and flooring logic correctly. I've added a few suggestions to improve code clarity by renaming some constants for better descriptiveness and using a template literal for a warning message to enhance readability. Overall, great work on this feature.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request rolls back a previous change and introduces a more straightforward approach for handling scheduled function timeouts. It synchronizes the Cloud Scheduler's attemptDeadline with the function's timeoutSeconds, applying a floor of 180s and a cap of 1800s. The changes are consistently applied across the codebase, removing the user-configurable attemptDeadlineSeconds and updating interfaces, validation, and tests accordingly. A helpful warning has also been added for when a function's timeout exceeds the maximum deadline. The implementation is solid, and I have one minor suggestion to improve code readability.
Rollback #9464
Firebase CLI will now synchronizes Cloud Scheduler's attemptDeadline with the function's timeoutSeconds for v2 scheduled functions, capped at 1800 seconds.
This is a change from original attempt where we tried to expose a new configuration for scheduled function that allowed users to directly set the attemptDeadline. We don't know why they would ever drift, so we'll simplify make sure the Scheduler's timeout is in sync with functions timeout.
Note that Cloud Scheduler has an attempt deadline range of [15s, 1800s], and defaults to 180s. We floor at 180s to be safe, even if the function timeout is shorter.
This is because GCF/Cloud Run will already terminate the function at its configured timeout, so Cloud Scheduler won't actually wait the full 180s unless GCF itself fails to respond. Setting it shorter than 180s might cause premature retries due to network latency.