-
Notifications
You must be signed in to change notification settings - Fork 161
Add job ID to error table #2245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
86652c5
to
f6acb98
Compare
…`job_id` and rename columns Pass through an `Option<u32>` for job id to record errors fix postgres-to-sqlite fix insert statements for job_id more fixes for postgres-to-sqlite fixes for sqlite-to-postgres sqlite does not support `SERIAL` add an id column for the error converter add an job_id column for the error converter
f6acb98
to
148f177
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Could you please update the docs MD file too?
collector/src/bin/collector.rs
Outdated
// not with a benchmark. | ||
conn.record_error( | ||
artifact_row_id, | ||
&format!("job:{}", benchmark_job.id()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can change the context now, because the job ID will be stored explicitly in the table now. Not sure what the context should be named in this case though, maybe just "unknown" or "general".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use something more general please as the context, retry count exhaustion is not the only thing that can happen, even now we have four different kinds of permanent errors. Maybe just Job failure, to distinguish it from benchmark failures.
…le and remove `IF NOT EXISTS`
I'm not sure which docs file you are referring too? |
|
7801679
to
f2ece09
Compare
I thought I'd updated it? I've changed |
Your updates are fine - I wrote that message before you did the MD docs update 😅 Maybe you just some some outdated GitHub comment? I'm fine with the current state of the PR :) Tried it locally and it seems to work fine. |
job_id
to the error table, remove unique constraint.benchmark
anderror
tocontext
andmessage
.job_id
in collector code.