-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat(backfill): add #5
Conversation
shekhirin
commented
Jul 18, 2024
•
edited
Loading
edited
4ef34b4
to
897fad9
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.
this is pretty cool!
one suggestion
backfill/src/main.rs
Outdated
Some(range) = self.backfill_rx.recv() => { | ||
let _ = self.backfill(range).await.inspect_err(|err| error!(%err, "Backfill error occurred")); |
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 effectively wait until the range is done, and could starve notification processing, we should just spawn this, ideally we also spawn and use some semaphor as guard so we limit the concurrent backfill requests
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.
last nit
let permit = self | ||
.backfill_semaphore | ||
.clone() | ||
.try_acquire_owned() | ||
.map_err(|err| eyre::eyre!("concurrent backfills limit reached: {err:?}"))?; |
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 needs to be moved into spawn and await acquire
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.
it will just block the RPC request, right? I explicitly return an error if we ran out of permits.
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 see
we'll also want a stop backfill command i think by job id, and returning a job id on creation of a backfill job |
yep, in a separate PR |