-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
chore(repo): Simplify health indicators interface #5404
Conversation
Remove isActive method as it's currently not in use.
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for novu-design ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@SokratisVidros |
@djabarovgeorge, I couldn't find any usages for the current status of the code. Would taking into account the paused state of each queue in the |
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.
I think we can remove the unused code since it's not relevant anymore. Looking at it again I think there might also be a flaw with the "is active" implementation anyway.
Here are a few things to consider:
- Previously, pm2 wouldn't stop the process if an error were thrown during startup. We fixed this by adding
process.exit(1);
, but I'm not sure if it's working as expected. - I believe the health checks in ECS run for 10 minutes. This interval could be shortened if needed.
@djabarovgeorge health check run every 10 seconds. Any other objections before merging this for now? |
@SokratisVidros Let's merge it! :) Sorry, I meant the worst-case scenario where the service is down, looking at it again looks like the health check could take up to 150 seconds, which seems more reasonable the 10 minutes. |
What change does this PR introduce?
Remove the
isActive
method from theIHealthIndicator
interface, as it's currently not in use.Why was this change needed?
Just a clean up to simplify things.