Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a new /health endpoint to the server. The review suggests upgrading this to a 'deep' health check that monitors the background processing thread and task manager status, while also recommending the use of 'async def' for the endpoint to improve performance.
| @self.app.get("/health") | ||
| def health_check(): | ||
| return {"status": "ok"} |
There was a problem hiding this comment.
The current health check is 'shallow' and only returns a static 'ok' status. In a distributed system with background workers, it is a best practice to provide a 'deep' health check that reflects the actual state of the service, such as whether the background processing thread is alive and the current status of the task queue. Additionally, using async def is preferred for simple endpoints in FastAPI to avoid the overhead of the external thread pool.
| @self.app.get("/health") | |
| def health_check(): | |
| return {"status": "ok"} | |
| @self.app.get("/health") | |
| async def health_check(): | |
| # Check if the background processing thread is still running | |
| is_alive = self.processing_thread is not None and self.processing_thread.is_alive() | |
| return { | |
| "status": "ok" if is_alive or self.processing_thread is None else "unhealthy", | |
| "worker_alive": is_alive, | |
| **task_manager.get_service_status() | |
| } |
No description provided.