-
Notifications
You must be signed in to change notification settings - Fork 168
Remove most of the old queue system #2360
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
|
Feels a tad risky however we may also want to add some |
|
I don't think that's needed tbh, having the old data in the production DB doesn't really hurt anything. If we'd ever do that I would only do it after a much longer timeframe. Reverting a PR is easy, reverting deleting a lot of DB data isn't :) |
|
|
||
| async fn last_n_artifact_collections(&self, n: u32) -> Vec<ArtifactCollection>; | ||
|
|
||
| // TODO: the following two functions do not work anymore and should not be used! |
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.
What blocks removal?
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 new system still uses those two functions to do some lookups if something exists in the old system. It should be removed, but I wanted to avoid changing the new system in this PR, to keep it almost strictly non-functional, as it's already quite big.
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.
| #[allow(unused)] | ||
| include: Option<&'a str>, | ||
| #[allow(unused)] | ||
| exclude: Option<&'a str>, |
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.
Presumably we stopped supporting these - do we intend to re-add support? Or should this be FIXME + deleted in the future?
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 didn't ever see anyone using them, and now that the whole benchmark takes just 40 minutes, and we can select subprofiles to make that even shorter, I don't think it's needed. So yeah, I wanted to remove them in the future, but I also wanted to mostly avoid functional changes in this PR, and just delete stuff. I'll do it in a follow-up.
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.
Jamesbarford
left a comment
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.
LGTM 👍
|
Ok, let's try if this doesn't break anything. I'll remove the rest in a follow-up. |
The new job queue system has been running in production, even with two parallel machines, for several weeks now, without any major issues. I don't expect that we would want to switch to the old system now; if some issues come up, we will just deal with them in the new system.
This PR thus removes most of the old job queue system from rustc-perf. There are still a few pieces left that are still used even with the new system, I plan to remove those later.
CC @Mark-Simulacrum