Deprioritize older releases when new ones are added and refactor get_new_crates method #3065
Conversation
github-actions
bot
added
the
S-waiting-on-review
| WHERE | ||
| name = $2 | ||
| AND version != $3 | ||
| AND attempt < $4 |
There was a problem hiding this comment.
If I understood the code correctly judging by other existing queries, this is basically a check for failed builds - is my understanding correct?
There was a problem hiding this comment.
Yes, partially.
we have certain "pre-build" errors where we have these re-attempts.
Example:
a workspace is released where the crates depend on each other. We don't know in which order they come into the build queue, so we just retry later for certain failures.
"normal" build failures (compile errors) don't stay in this table.
( all of that being said, I'll think longer about us removing all builds from the queue, I have a hunch that this is because of some legacy limitation we don't have any more)
| debug!( | ||
| "{}-{} added into build queue", | ||
| release.name, release.version | ||
| ); |
There was a problem hiding this comment.
This log was already existing in the previous implementation with string interpolation - even though this is tracing::debug!, not log::debug! based on what I learned in my previous PR - is there any reason as for why we use string interpolation here, or can I change it like this (as well as in process_version_deleted)?
| debug!( | |
| "{}-{} added into build queue", | |
| release.name, release.version | |
| ); | |
| debug!( | |
| name=%release.name, | |
| version=%release.version, | |
| "added into build queue" | |
| ); |
There was a problem hiding this comment.
legacy code, feel free to update it
Carbonhell
force-pushed
the
tweak/deprioritize-older-releases
branch
from
fa532b0 to
744ab80
Compare
refactor(queue): break up get_new_crates() in smaller methods to facilitate testing
Carbonhell
force-pushed
the
tweak/deprioritize-older-releases
branch
from
744ab80 to
308726c
Compare
| self.queue_crate_invalidation(krate).await; | ||
| self.remove_crate_from_queue(krate).await?; | ||
| continue; | ||
| if self.process_change(index, &mut conn, change).await? { |
There was a problem hiding this comment.
in the old code we continued to handle other changes in case of an error, even when handling one change failed.
The new code actually stops handling changes.
We need the old behaviour back.
2 participants
Closes #3064 .
To facilitate testing, I decided to split up the
get_new_cratesmethod in smaller units. The actual change that updates the priority for older releases is contained in thedeprioritize_other_releasesmethod, which is called when only a new crate is processed. This logic is only invoked when processing new crate versions are found by the registry watcher.For the refactor, a point worthy of note relates to the removal of manual
report_errorcalls, in favor of propagating the error to the caller, since they're already reported in the registry watcher entrypoint - this does not include calls todeprioritize_other_releasesandqueue_crate_invalidation, for which a failure can be logged without killing the watcher.The tests introduced are quite simple - let me know if more are needed!