-
Notifications
You must be signed in to change notification settings - Fork 32
rofl-scheduler: Add initial ROFL scheduler #2179
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
✅ Deploy Preview for oasisprotocol-oasis-sdk canceled.
|
f214f75
to
11c28a4
Compare
fe07946
to
1068e66
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2179 +/- ##
==========================================
- Coverage 58.90% 54.10% -4.81%
==========================================
Files 151 156 +5
Lines 11333 12123 +790
==========================================
- Hits 6676 6559 -117
- Misses 4657 5564 +907 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
203ea1a
to
d7d878f
Compare
496f7b8
to
ce6d4df
Compare
ce6d4df
to
e806c6d
Compare
64000d2
to
47e618e
Compare
f329364
to
ebd4a1c
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.
Left few comments, will have another look once we merge the other PR.
[package] | ||
name = "rofl-scheduler" | ||
version = "0.1.0" | ||
edition = "2021" |
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.
2021?
#[default] | ||
#[serde(skip)] | ||
#[cbor(skip)] | ||
Invalid, |
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.
Is this necessary?
b9e5264
to
ced75bb
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.
First pass, the main scheduler loop looks sound!
"id" => ?instance.id, | ||
"creator" => instance.creator, | ||
); | ||
local_state |
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.
Is this step needed? Same below.
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.
Yes, otherwise the unaccepted instances will never be removed. The idea is that scheduler instances will fuzzily remove instances they reject. So if no scheduler instance accepts an instance it will eventually be removed.
}) | ||
.collect(); | ||
|
||
for instance in instances { |
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.
Should we also check if the instance is paid?
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.
When you create the instance you must also pay the initial term (this is enforced by the marketplace). Otherwise the instance will not even be created. The scheduler has some time to accept an instance, otherwise it can be cancelled and refunded.
d2b69ac
to
99163cc
Compare
99163cc
to
b4f876c
Compare
slog::error!(self.logger, "task failed"; "err" => ?err); | ||
} | ||
Ok(Ok(_)) => { | ||
// Ok. |
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.
Should we add some debug logging here for each finished task? Might be helpful for debugging in case there would be issues with specific tasks executing/blocking for long time?
c0b4f24
to
80b66b6
Compare
80b66b6
to
f89a887
Compare
…ostko/feature/rofl-scheduler rofl-scheduler: Add initial ROFL scheduler 28069c3
…/kostko/feature/rofl-scheduler rofl-scheduler: Add initial ROFL scheduler 28069c3
…sisprotocol/kostko/feature/rofl-scheduler rofl-scheduler: Add initial ROFL scheduler 28069c3
…oasisprotocol/kostko/feature/rofl-scheduler rofl-scheduler: Add initial ROFL scheduler 28069c3
Fixes #2173