[V1][Frontend] Improve Shutdown And Logs #14048
Conversation
Signed-off-by: [email protected] <[email protected]>
… handle properly Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
robertgshaw2-redhat
added
the
ready
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
| await self.abort(request_id) | ||
| if self.log_requests: | ||
| logger.info("Request %s failed.", request_id) | ||
| logger.exception("GOT EXCEPTION:", exc_info=e) |
There was a problem hiding this comment.
Is this line meant to be here? We should probably change the message if so, and there's no need to set exc_info=e since it will use that by default in the except block.
| except Exception as e: | ||
| await self.abort(request_id) | ||
| if self.log_requests: | ||
| logger.info("Request %s failed.", request_id) |
There was a problem hiding this comment.
Maybe add the exception message to the log line too? (but still single line)
| if self.output_handler is None: | ||
| return True | ||
|
|
||
| return not self.output_handler.done() |
There was a problem hiding this comment.
| if self.output_handler is None: | |
| return True | |
| return not self.output_handler.done() | |
| return self.output_handler is None or not self.output_handler.done() |
| @property | ||
| def errored(self) -> bool: | ||
| return False | ||
| return (self.engine_core.is_engine_dead or not self.is_running) |
There was a problem hiding this comment.
nit:
| return (self.engine_core.is_engine_dead or not self.is_running) | |
| return self.engine_core.is_engine_dead or not self.is_running |
| except Exception as e: | ||
| logger.exception("EngineCore got an Exception during startup:", | ||
| exc_info=e) | ||
| ready_pipe.send({"status": "FAILED"}) | ||
| raise e | ||
|
|
||
| finally: | ||
| ready_pipe.close() |
There was a problem hiding this comment.
I'm not sure that this should go here. Probably better to catch/handle in the calling method, including handling sending the pipe ready message since it "owns" the pipe. In the failure case, I'm not sure that we need to send failed status back since the process could die for other reasons without us being able to do so, and we need to handle that equivalently from the client side.
| @staticmethod | ||
| def wait_for_ready( | ||
| unready_proc_handle: UnreadyWorkerProcHandle) -> WorkerProcHandle: |
There was a problem hiding this comment.
why not a method on UnreadyProcHandle?
|
@robertgshaw2-redhat, can I help out with a refactor of this one again? |
|
This pull request has merge conflicts that must be resolved before it can be |
|
Back to #11737 |
In place of #11737
authored by: @robertgshaw2-redhat
SUMMARY:
DESIGN:
for errors during startup, we wrap init code with try...catch and push FAILED over the ready PIPE. This works well since the parent processes are waiting for confirmation
for errors during runtime, we wrap the busy loops with try..catch and push failure messages over the existing IPC mechanisms.
One weakness is that issues with the ipc mechanisms themselves are not handled explicitly. We will explore this more in a follow up
TEST MATRIX:
Fixes: #12690