Update celery config #253
Update celery config #253
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes modify the Celery application configuration, the authentication signup response, and the supervisord command used to start the Celery worker. In the Celery configuration, several parameters have been added or adjusted to improve task distribution, resource management, and robustness (e.g., task acknowledgment, memory limits, and rate limiting). The authentication module now returns an extra flag indicating whether a user already exists. The supervisord configuration has been updated to pass additional arguments to the Celery worker command for memory and task management. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant AuthAPI as AuthAPI Service
participant DB as Database
Client->>AuthAPI: POST /signup request
AuthAPI->>DB: Query for user existence
alt User exists
DB-->>AuthAPI: Return existing user record
AuthAPI-->>Client: {"uid": user_id, "exists": true}
else New user
DB-->>AuthAPI: Return no record
AuthAPI->>DB: Create new user
DB-->>AuthAPI: Return new user record
AuthAPI-->>Client: {"uid": new_user_id, "exists": false}
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/celery/celery_app.py (1)
57-62: Verify rate limit and timeout settings.The configuration introduces several important limits:
- Rate limit of 10 tasks per minute might be too restrictive
- Visibility timeout (45 minutes) should align with task_time_limit (90 minutes)
Run this script to analyze task execution patterns:
Also, consider these architectural recommendations:
- Monitor task queue length to ensure rate limiting isn't causing backlog
- Consider implementing exponential backoff for retries
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Analyze task execution patterns # Search for task timeouts rg "TimeLimitExceeded|SoftTimeLimitExceeded" # Search for rate limit hits rg "Rate limit exceeded|RateLimitExceeded"Length of output: 91
Manual Verification Needed for Task Limits and Timeouts
The shell script produced no output, so we couldn’t automatically confirm issues with rate limits or timeouts. It’s recommended that you manually verify these settings (i.e., check if the task time limits are configured as expected and that the rate limiting isn’t disrupting workload processing, and confirm that the visibility timeout is aligned with the intended task timeout). You might also consider searching the codebase for any references to
task_time_limitto ensure the 45-minute visibility timeout properly pairs with a 90-minute task execution limit.app/modules/auth/auth_router.py (2)
55-58: Consider using JSONResponse for consistency.The response uses
Responsewith manual JSON serialization. Consider usingJSONResponsefor consistency with other endpoints.- return Response( - content=json.dumps({"uid": uid, "exists": True}), - status_code=200, - ) + return JSONResponse( + content={"uid": uid, "exists": True}, + status_code=200 + )
90-93: Use consistent response handling.Similar to above, consider using
JSONResponsehere as well.- return Response( - content=json.dumps({"uid": uid, "exists": False}), - status_code=201, - ) + return JSONResponse( + content={"uid": uid, "exists": False}, + status_code=201 + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/celery/celery_app.py(1 hunks)app/modules/auth/auth_router.py(2 hunks)deployment/prod/celery/celery-api-supervisord.conf(1 hunks)
🔇 Additional comments (2)
deployment/prod/celery/celery-api-supervisord.conf (1)
6-6: Verify memory limit and task settings.The added parameters look good but require verification:
--max-memory-per-child=2000000(2GB) should be sufficient for your workload--max-tasks-per-child=200matches the configuration in celery_app.py--optimization=fairis appropriate for your use caseRun this script to check memory usage patterns of your Celery workers:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check memory usage patterns in logs # Search for OOM (Out of Memory) incidents rg -i "killed|out of memory|oom" # Search for memory-related warnings rg -i "warning.*memory|error.*memory"Length of output: 334
Memory and Task Settings Verified
- The search logs did not reveal any OOM incidents or memory-related warnings.
- The specified parameters in the supervisor configuration match those in the
celery_app.pyfile.- The 2GB memory limit and task restart setting appear appropriate for the workload.
app/celery/celery_app.py (1)
52-56: LGTM! Task distribution optimizations look good.The changes improve task distribution reliability:
worker_prefetch_multiplier=1prevents task hoardingtask_acks_late=Trueensures proper task completiontask_track_started=Truehelps with task monitoring
|
Summary by CodeRabbit
New Features
Chores