Deregister services from service registry on shutdown #5396
Conversation
pull-request-size
bot
added
the
size/M
There was a problem hiding this comment.
@khushboobhatia01 Good start.
- Is it possible to write a unit test for deregister_service?
- How about other services (i.e. api, rulesengine, etc.)? Search service_registry=True to find them.
pull-request-size
bot
added
size/L
Addressed @m4dcoder |
…ister_on_shutdown
…st2 into deregister_on_shutdown
| try: | ||
| service_setup.deregister_service(service) | ||
| except: | ||
| assert False, "service_setup.deregister_service raised exception" |
There was a problem hiding this comment.
@khushboobhatia01 What happens if this error is raised on shutdown? I think it is ok to make this silent if there is nothing to be deregister.
There was a problem hiding this comment.
@m4dcoder We don't expect deregister() to raise an exception when service_registry is disabled or there's nothing to deregister. This test case would fail only when some unintended change is causing deregister to fail. All edge cases are handled in deregister(). Therefore it might be good to know if some new change is causing it to fail.
There was a problem hiding this comment.
@khushboobhatia01 Could you please also add Changelog for this PR before we merge it?
…ister_on_shutdown
Done @armab. Thanks for reminding :) |
|
Great stuff! Thanks @khushboobhatia01 for the work and @m4dcoder for review! 👍 |
Few services like worker, workflow engine, scheduler and notifier perform a shutdown sequence before exiting the process. This PR will ensure that the services deregister themselves from service registry on shutdown and then continue with shutdown sequence.
(PR based on #5373 step 1)