Proto changes for showing ExecuteResponse in the UI #5502
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bduffany
requested review from
tylerwilliams,
iain-macdonald and
vadimberezniker
| // [build.bazel.remote.execution.v2.ExecuteResponse] produced by the executor | ||
| // for the latest attempt of this execution. | ||
| // | ||
| // This digest can be used to fetch a specially formatted ActionResult from |
There was a problem hiding this comment.
Kind of ambivalent here, but did you consider putting this in the CAS instead as the hashed(executeResponse) minus this field?
There was a problem hiding this comment.
Yeah, I figured the downside of that approach is that we have to store the digest somewhere, whereas this approach would just hash the execution_id that we already store - similar to how we add the invocation ID to the Action digest today for failed actions. I am hesitant to add more columns to the Executions table but if you don't think it's a big deal, we can do that instead.
vadimberezniker
approved these changes
Dec 11, 2023
tylerwilliams
approved these changes
Dec 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the proto changes needed to show
ExecuteResponsein the action page instead ofActionResult. ShowingExecuteResponsehas two advantages:ExecuteResponsefor the execution ID, rather than the latestActionResultassociated with the action digest. The latter is inaccurate/incorrect and often confusing.ExecuteResponsecontains a superset of info, such as:statusfield (execution error details when the task either fails to start, or terminates abnormally), which is not representable by an ActionResult.server_logsfield which allows attaching arbitrary logs. We can use this for attaching firecracker VM logs.Note, this does NOT take any steps towards addressing https://github.com/buildbuddy-io/buildbuddy-internal/issues/2918 which is a bit more involved as it will likely involve DB schema changes.
Related issues: