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
redo the notify events between eval and queue-runner #645
base: master
Are you sure you want to change the base?
redo the notify events between eval and queue-runner #645
Conversation
(cherry picked from commit 73ca325)
|
||
pqxx::result res = txn.parameterized | ||
("select id, project, jobset, job, drvPath, maxsilent, timeout, timestamp, globalPriority, priority from Builds " | ||
"where id IN (select build from jobsetevalmembers where eval = $1) order by globalPriority desc, 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.
This looks like it could be a fairly simple join, and avoid a temporary table.
from clever on IRC:
|
@@ -74,6 +75,53 @@ struct PreviousFailure : public std::exception { | |||
}; | |||
|
|||
|
|||
bool State::getQueuedBuildsInEval(Connection & conn, | |||
ref<Store> destStore, unsigned int evalId) |
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.
There is a lot of code duplication between this and getQueuedBuilds
, maybe that can be avoided?
If I understand correctly what this does, it could cause a lot of extra load on hydra.nixos.org, since for every Nixpkgs/NixOS evaluation, it will have to process every finished build again and run plugins. That's tens of thousands of builds per evaluation... |
yeah, it may be best to not upstream this until the hydra-notify perf issues are fixed, possibly as a result of redoing the whole notify system |
FYI, I've rewritten this to work with recent Hydra versions here: https://github.com/hackworthltd/hydra/commit/f9bf7f6f4982ef35fd64d0c6f88a96a182de5dff We've been using it in production for awhile and it seems to work reasonably well, with one caveat that I'll describe below. I dunno why IOHK originally wrote this patch (something to do with performance?), but the reason we use it is as follows:
As the user, there is no way to know whether the remaining 15 jobs are still running, the reporting failed, or what, unless you go check Hydra. This is not very nice UX, as normally you only need to check Hydra when a job fails and you want to see the logs. So, this patch fixes the problem by ensuring that all 20 job statuses are reported in this scenario, and that's why we use it. There is one problem, however. This patch (or at least my rewritten version — I'm not sure whether this issue exists in @cleverca22's original patch) will report all cached statuses for the given job to GitHub. So, for example, if a particular job has been cached for the last 4 commits to However, if you want to do something like kick off a GitHub Action workflow whenever a job named So while I'm not requesting that this particular patch should be merged, it would be nice if one could configure a Hydra to report cached status updates to GitHub when a jobset is built (and just once, rather than (There is currently no GitHub Action plugin for Hydra, but we have a very hacky one here, which is how I discovered this wrinkle in the eval patch: https://github.com/hackworthltd/hydra/commit/a5ce9dce8d40ded1b535e31f3db418e7795aa226) |
Two more things:
|
(cherry picked from commit 73ca325)
We did this at IOHK to improve some performance problems with notifications. It's still not perfect, and there may be a good reason to not do it this way, but creating a PR for the community to discuss whether this is worthwhile, and if so if it needs improvements to get in.