Feat: Make functions worker customizable#10287
Conversation
📝 WalkthroughWalkthroughThis change introduces new resources named Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested reviewers
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/init/resources.php (1)
106-109: Injection list mismatch (pre-existing bug)
consumerStatsUsagereturns$consumerbut declares['publisher']as its only dependency.
Switch to['consumer']to avoid accidental container mis-resolves.
🧹 Nitpick comments (4)
app/init/resources.php (1)
86-88: Consider wiring queueForFunctions to this new pool too
publisherFunctionsis registered butqueueForFunctions(Line 140) still injects the genericpublisher.
For clarity and future config isolation, wirequeueForFunctionstopublisherFunctionsinstead.app/cli.php (1)
197-199: Resource added, but queues still use generic publisherNice addition. Consider also switching the
queueForFunctionsresource (Line 212-214) to injectpublisherFunctionsfor consistency with the scheduler changes.app/worker.php (1)
254-256: Publisher alias added – check downstream wiringSame note: downstream
queueForFunctionsstill pulls the genericpublisher(Line 322-325). Aligning it withpublisherFunctionswill complete the separation.app/controllers/api/health.php (1)
689-695: Optional: unify naming to 'publisherFunctions' for consistencyOther places (e.g., platform tasks) use
publisherFunctions. Consider renaming this injection to reduce cognitive load.Apply locally if the DI container uses
publisherFunctions:- ->inject('publisherForFunctions') + ->inject('publisherFunctions') @@ - ->action(function (int|string $threshold, Publisher $publisherForFunctions, Response $response) { + ->action(function (int|string $threshold, Publisher $publisherFunctions, Response $response) { @@ - $size = $publisherForFunctions->getQueueSize(new Queue(Event::FUNCTIONS_QUEUE_NAME)); + $size = $publisherFunctions->getQueueSize(new Queue(Event::FUNCTIONS_QUEUE_NAME));If
publisherForFunctionsis the intended canonical name, consider adding an alias forpublisherFunctionsin resources to match usage elsewhere.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/cli.php(1 hunks)app/controllers/api/health.php(1 hunks)app/init/resources.php(2 hunks)app/worker.php(2 hunks)src/Appwrite/Platform/Tasks/ScheduleBase.php(3 hunks)src/Appwrite/Platform/Tasks/ScheduleExecutions.php(1 hunks)src/Appwrite/Platform/Tasks/ScheduleFunctions.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Appwrite/Platform/Tasks/ScheduleFunctions.php (1)
src/Appwrite/Utopia/Response/Model/Func.php (1)
Func(8-207)
src/Appwrite/Platform/Tasks/ScheduleExecutions.php (1)
src/Appwrite/Utopia/Response/Model/Func.php (1)
Func(8-207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (7)
app/init/resources.php (1)
101-103: Symmetry looks good👍
consumerFunctionsmirrors the existing pattern and lists the correct dependency.src/Appwrite/Platform/Tasks/ScheduleFunctions.php (1)
93-94: Verify$this->publisherFunctionsis initialisedThe new
Func($this->publisherFunctions)assumesScheduleBasenow stores that property.
If any subclass calls this beforeScheduleBase::action()injects it, a fatal error will occur. Please double-check initialisation.src/Appwrite/Platform/Tasks/ScheduleExecutions.php (1)
33-34: Same initialisation concernEnsure
$this->publisherFunctionsis always populated; otherwise coroutine startup will crash.app/worker.php (1)
274-276: Consumer alias looks correct
consumerFunctionsmirrors existing pattern. No issues spotted.src/Appwrite/Platform/Tasks/ScheduleBase.php (3)
29-29: Property added for function-specific publisher looks goodTyped BrokerPool property is consistent with existing
$publisherand$publisherMigrations. No issues spotted.
45-50: Dependency ‘publisherFunctions’ correctly registered in DI
ThepublisherFunctionsresource is explicitly registered via App::setResource('publisherFunctions', …) in app/init/resources.php (and mirrored in app/cli.php and app/worker.php), so the->inject('publisherFunctions')in ScheduleBase.php will resolve without issue.
72-80: Action signature update is safe – no subclass overrides
The addedBrokerPool $publisherFunctionsparameter and its assignment match the DI order and won’t break existing code: none of the threeScheduleBasesubclasses overrideaction(). No further changes are needed.
✨ Benchmark results
⚡ Benchmark Comparison
|
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist