-
Notifications
You must be signed in to change notification settings - Fork 161
Fix; job_queue
insert only if tag exists in benchmark_request
#2254
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
Fix; job_queue
insert only if tag exists in benchmark_request
#2254
Conversation
database/src/pool/postgres.rs
Outdated
.conn() | ||
.query_one( | ||
"SELECT tag FROM benchmark_request WHERE tag = $1", | ||
&[&benchmark_request.parent_sha()], |
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.
If the parent SHA is NULL (which will happen for try builds), tag = NULL
will return NULL
, which will be interpreted as false
, right? Then the query_one
call will fail.
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.
The same would happen if the parent SHA was actually missing in the DB. We'll need to use e.g. query
, not query_one
.
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.
Judging by a raft of tests failing 😓. Yes.
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.
I don't know if I like what I have done here but it does achieve what we want and should only be temporary; 68d0d87
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.
This will most likely mean that all parent_sha
's will be null when initially starting the CRON. At least this is what I have observed locally.
68d0d87
to
1acbb5b
Compare
Thinking about this a bit more, it would be better if this change was actually not only temporary; ideally, the system should be able to bootstrap itself even if we restart/redeploy it on some other server, or if we run it locally, otherwise we couldn't easily test the new job queue locally if it would be throwing the FK errors all the time. So we should always count with the possibility that the parent commit might be missing. I didn't like the option of modifying the benchmark request first, and wanted to only modify the creation of jobs, but after some experiments I did on the code, I guess it kind of makes sense. The master commit might not in fact have a parent available, because the git tree has to stop somewhere, so representing it by making the master parent SHA nullable kinda makes sense. We should make use of the benchmark request index though. Rather than doing an extra SQL query, we can iterate the master commits in reverse order (from the oldest to the newest), and when we run into a request whose parent is not in the index, we will forcefully set the parent SHA to |
database/src/pool.rs
Outdated
let db = ctx.db(); | ||
|
||
// Releases don't need parent shas | ||
ctx.insert_release_request("parent-sha-1").await; |
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 don't currently check against it, but a release request cannot be the parent of anything. So I'd rather not have this situation in tests.
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, looks good! (Apart from CI failures, ofc xD)
c253a78
to
fbba36c
Compare
This PR is superseded by; #2255 |
Make the parent sha null if it does not refer to a tag in the
benchmark_request
table. This prevents a foreign key violation when subsequently trying to create jobs for back filling.