-
-
Notifications
You must be signed in to change notification settings - Fork 11
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: adds persistence layer for app state and job results [SBK-363] #45
Conversation
SBK-363 "feat: adds persistence layer for app state and job results" (ApeWorX/silverback #45)
WIP: This is still pretty raw, looking for early feedback. Will leave some personal notes inline. What I didAdds persistence for app state and job results Closes #33 How I did itAdds mongo support by way of beanie. Stores block numbers the runner sees in an app state collection. Stores handler job results in result collection. How to verify itRun it with Checklist
ApeWorX/silverback #45 by mikeshultz on GitHub via LinearSync |
silverback/persistence.py
Outdated
) | ||
|
||
|
||
class BasePersistentStorage(ABC): |
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 there anything we need to add for v1? This is bare minimum for what this PR needs but we could future-think here as well for aggregate data or API fetching.
Here's what the results docs look like in MongoDB:
And app state:
|
… network settings
Co-authored-by: El De-dog-lo <[email protected]>
def do_something_on_startup(startup_state): | ||
... # Reprocess missed events or blocks | ||
""" | ||
return self.broker.task(task_name="silverback_startup") |
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 are the pros/cons of moving the startup/shutdown handlers into separate tasks from the worker startup/shutdown events
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.
They cannot be combined. They're very separate things.
Having on_startup
be a worker event means unexpected duplication of execution. Having a separate task also allows us to feed the handler useful information, like last_block_*
. This is basically what we need for ApePay.
Renaming the previous handlers to be explicit worker event handlers implies their actual usage and is clearer, IMO. They're two distinct things.
The only real con is we're changing up behavior here. Anyone setting up worker state may need to refactor.
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.
They cannot be combined. They're very separate things.
Having
on_startup
be a worker event means unexpected duplication of execution. Having a separate task also allows us to feed the handler useful information, likelast_block_*
. This is basically what we need for ApePay.Renaming the previous handlers to be explicit worker event handlers implies their actual usage and is clearer, IMO. They're two distinct things.
The only real con is we're changing up behavior here. Anyone setting up worker state may need to refactor.
but won't this mean that only one worker will handle this event on startup? vs. the worker startup event is executed on every worker as it's set 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.
yes. on_startup
will only run once upon runner startup. on_worker_startup
will run on every worker startup.
… feat/persistence
|
||
silverback run "example:app" \ | ||
--network :mainnet:alchemy \ | ||
--runner "silverback.runner:WebsocketRunner" |
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.
question(non-blocking): should we standardize runner
to client
?
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.
Hard question, tbh. Would it be right to say "choose your client" to the user who wants to say use the WebsocketRunner?
) | ||
|
||
try: | ||
await self.persistence.add_result(handler_result) |
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.
random question: does the middleware act on the client or the worker?
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.
Both. pre_send and post_send are client side, and pre_execute, post_execute, and post_save are worker-side. It never comes back to the client after processing, though.
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.
suggestion: we find a different name rather than persistence
as it's a little awkward and not a good indicator of it's purpose
maybe data_store
or metrics
@@ -17,6 +18,9 @@ class Settings(BaseSettings, ManagerAccessMixin): | |||
testing or deployment purposes. Defaults to a working in-memory broker. | |||
""" | |||
|
|||
# A unique identifier for this silverback instance | |||
INSTANCE: str = "default" |
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's the idea behind this?
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 ability for multiple Silverback instances to use a single persistence backend. For ApePay, I'm setting it to apepay-sepolia, and we may have others for other networks. Otherwise, they'd step on each-others toes and confusingly share state.
Co-authored-by: El De-dog-lo <[email protected]>
Co-authored-by: El De-dog-lo <[email protected]>
I don't know. |
What I did
Adds persistence for app state and job results. Alters startup/shutdown handlers a bit.
Closes #33
How I did it
Adds persistent storage support of state and results. Stores block numbers the runner sees in an app state collection. Stores handler job results in result collection.
How to verify it
Run it with these defined:
Checklist