jobs,jobspb: rename ExecutionLog, populate #69370
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
ajwerner
requested review from
a team and
rhu713
and removed request for
a team
|
This change is |
fqazi
approved these changes
Aug 25, 2021
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 14 of 14 files at r2, 15 of 15 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rhu713 and @sajjadrizvi)
sajjadrizvi
approved these changes
Aug 25, 2021
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 14 of 14 files at r2, 15 of 15 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status:
pkg/jobs/config.go, line 69 at r4 (raw file):
// defaultExecutionErrorsMaxEntrySize is the maximum allowed size of an // error. If this size is exceeded, the error will formatted as a string
nit: s/ will formatted/will be formatted
Release justification: non-production code changes Release note: None
ajwerner
force-pushed
the
ajwerner/execution-errors-in-jobs
branch
2 times, most recently
from
7649f96
to
d52689c
Compare
Also moves the code to a new file. Release justification: category 2 Release note: None
ajwerner
force-pushed
the
ajwerner/execution-errors-in-jobs
branch
from
d52689c
to
0c9833b
Compare
ajwerner
changed the title
ajwerner
force-pushed
the
ajwerner/execution-errors-in-jobs
branch
4 times, most recently
from
d762bcb
to
4825d71
Compare
I got cold feet on the `ExecutionLog` in the `Payload` being populated at the beginning of job execution. I become uncomfortable with the idea of writing to the `Payload` an extra time for every job execution in the happy case. These things can get big. Instead we only populate events when a retriable error occurs. Given that, the events have been renamed to `RetriableExecutionError`. This data is exposed in the crdb_internal.jobs table via the `execution_errors` string array. The decision to use an array of strings instead of json was predicated upon the idea that these errors are for human consumption. The structured data continues to exist in the protobuf. Work to truncate these logs will come in the next commit. Release justification: bug fixes and low-risk updates to new functionality. Release note (general change): When jobs encounter retriable errors during execution, they will now record these errors into their state. The errors as well as metadata about the execution can be inspected via the newly added `execution_errors` field of `crdb_internal.jobs` which is a `STRING[]` column.
ajwerner
force-pushed
the
ajwerner/execution-errors-in-jobs
branch
from
4825d71
to
870ff19
Compare
This change limits the size of execution error entries based on cluster settings (which are not public). Release justification: low risk change to new functionality Release note: None
ajwerner
force-pushed
the
ajwerner/execution-errors-in-jobs
branch
from
870ff19
to
b7301c6
Compare
|
bors r+ |
|
Build succeeded: |
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.
I got cold feet on the
ExecutionLogin thePayloadbeing populated at thebeginning of job execution. I become uncomfortable with the idea of writing to
the
Payloadan extra time for every job execution in the happy case. Thesethings can get big. Instead we only populate events when a retriable error
occurs. Given that, the events have been renamed to
RetriableExecutionError.This data is exposed in the crdb_internal.jobs table via the
execution_errorsstring array. The decision to use an array of strings instead of json was
predicated upon the idea that these errors are for human consumption. The
structured data continues to exist in the protobuf.
These events are retained in the
PayloadinRetriableExecutionErrorLog.A finite number of events are retained, and that number is controlled by the new
non-public cluster setting
jobs.execution_errors.max_entries. The size of theevents is also controlled by a cluster setting:
jobs.execution_errors.max_entry_size.If an event would be larger than this size, the error it would contain is formatted
to a string and truncated to the max size. This loses structure but at least retains
some observability.
Fixes #68800.
Release justification: bug fixes and low-risk updates to new functionality.
Release note (general change): When jobs encounter retriable errors during
execution, they will now record these errors into their state. The errors as
well as metadata about the execution can be inspected via the newly added
execution_errorsfield ofcrdb_internal.jobswhich is aSTRING[]column.