[9.0.0] Inform ImportantOutputHandler of too large stdout/err. #28087
Conversation
PiperOrigin-RevId: 846883855 Change-Id: I8c34491974574704155cf888b3ef1d1ccf60195a
bazel-io
added
the
team-Configurability
bazel-io
added
awaiting-review
PiperOrigin-RevId: 846883855 Change-Id: I8c34491974574704155cf888b3ef1d1ccf60195a
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to inform the ImportantOutputHandler about stdout/stderr files that are too large to be displayed on the console. This ensures these files are treated as important outputs and are accessible, which is particularly useful in remote execution scenarios. The changes involve adding a new method to the ImportantOutputHandler interface, plumbing the maxStdoutErrBytes option from UiOptions down to the SkyframeActionExecutor, and implementing the logic to check output sizes and notify the handler. The changes look solid, but I have one suggestion to improve logging in an exception handler.
| logger.atWarning().withCause(e).log( | ||
| "Failure informing important output handler of stdout/stderr"); | ||
| } catch (InterruptedException e) { | ||
| logger.atInfo().log("Informing important output handler of stdout/stderr"); |
There was a problem hiding this comment.
The log message for the InterruptedException is misleading. It should state that the operation was interrupted. Also, logging this at INFO level might be too low for an interruption. For consistency with the IOException handler, consider using WARNING and including the exception cause.
| logger.atInfo().log("Informing important output handler of stdout/stderr"); | |
| logger.atWarning().withCause(e).log("Interrupted while informing important output handler of stdout/stderr"); |
|
This isn't relevant for bazel. |
2 participants
PiperOrigin-RevId: 846883855
Change-Id: I8c34491974574704155cf888b3ef1d1ccf60195a
Commit 44b6457