-
Notifications
You must be signed in to change notification settings - Fork 73
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
chore(workflows): move wf gc and metrics publish into worker #1943
base: 01-25-chore_restructure_server_binaries
Are you sure you want to change the base?
Conversation
Deploying rivet with Cloudflare Pages
|
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.
PR Summary
This PR consolidates workflow management by moving garbage collection and metrics publishing into the worker context. Here's a summary of the key changes:
- Moves workflow garbage collection and metrics publishing from standalone services into the Worker module with new
gc()
andpublish_metrics()
functions - Adds new key types
LastPingTsKey
andMetricsLockKey
for tracking worker instance health and coordinating metrics collection - Implements
clear_expired_leases()
andpublish_metrics()
in database adapters (CrdbNats and FdbSqliteNats) to handle workflow failover and metrics - Replaces
WORKER_ACTIVE
metric with more granularWORKER_LAST_PING
metric that tracks individual worker instance health - Removes standalone services and their configurations from edge-server and server run configs, consolidating functionality into the worker
The changes improve the architecture by centralizing workflow management functionality and providing better worker health tracking, though the removal of integration tests should be addressed.
19 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
packages/common/chirp-workflow/core/src/db/fdb_sqlite_nats/keys/worker_instance.rs
Show resolved
Hide resolved
packages/common/chirp-workflow/core/src/db/fdb_sqlite_nats/keys/worker_instance.rs
Show resolved
Hide resolved
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.
style: The old code used a 60 second operation timeout (Duration::from_secs(60)), but there's no explicit timeout in the new worker GC implementation. Consider adding a timeout to prevent long-running GC operations from blocking the worker.
packages/services/workflow/standalone/metrics-publish/src/lib.rs
Outdated
Show resolved
Hide resolved
bee5d38
to
df8b94d
Compare
e4b3b80
to
541aa12
Compare
df8b94d
to
f6f8a7b
Compare
541aa12
to
9b0bc5b
Compare
f6f8a7b
to
d7a614b
Compare
9b0bc5b
to
0e34b51
Compare
d7a614b
to
cbfb3c8
Compare
0e34b51
to
f436af7
Compare
cbfb3c8
to
f993bd7
Compare
f436af7
to
c8701a1
Compare
f993bd7
to
ca16fe8
Compare
Changes