Conversation
|
cc @yiranwang52 |
| print("Triggered gc.collect() on all workers.") | ||
|
|
||
|
|
||
| @cli.command(name="health-check", hidden=True) |
| if time_ok: | ||
| sys.exit(0) | ||
| else: | ||
| sys.exit(1) | ||
|
|
There was a problem hiding this comment.
should we sys exit or just throw an error/warning/print an appropriate message.
There was a problem hiding this comment.
I think it's fine. This is only meant for machine and we should think of this as a really annoying way of implementing a function that returns true or false depending on the status.
Concretely, printing in this will just generate lots of spam in the logs (since this will be polled all the time).
| # client creation or ping fails, we will still exit with a non-zero | ||
| # exit code. | ||
| redis_client.ping() | ||
| sys.exit(0) |
There was a problem hiding this comment.
is this supposed to only return exist statuses?
There was a problem hiding this comment.
Yeah it purely returns exit codes
|
Looks like there might be an edge case when starting with |
|
Is it straightforward to test the edge-case fix? |
|
I think I already did? The tests are looking good, i'll push a skip on windows, then let them run one more time. |
|
serve, rllib, and multi tenancy (also passes locally) tests are all known to be flaky. merging. |
Why are these changes needed?
(See RFC, this PR strictly follows it).
Related issue number
#15265
Checks
scripts/format.shto lint the changes in this PR.