Skip to content
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!: add app.state #80

Merged
merged 17 commits into from
Oct 9, 2024
Merged

feat!: add app.state #80

merged 17 commits into from
Oct 9, 2024

Conversation

fubuloubu
Copy link
Member

@fubuloubu fubuloubu commented May 5, 2024

What I did

This PR adds worker shared state (accessible via app.state) and adds 2 new system tasks to support loading and storing a snapshot of that managed state. Currently this is just last_block_processed and last_block_seen.

This paves the way for the Parameter feature, but does not include that just yet.

BREAKING CHANGE: state snapshot features migrates from runner to worker and must be triggered by runner

How I did it

How to verify it

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
    - [ ] New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

@fubuloubu fubuloubu requested a review from mikeshultz May 5, 2024 02:23
@fubuloubu fubuloubu changed the title feat: add app.state and parameters API feat!: add app.state and parameters API May 5, 2024
Copy link
Contributor

@mikeshultz mikeshultz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick initial pass. Kind of wish "state" wasn't used for so many different things, it's getting confusing.

silverback/application.py Show resolved Hide resolved
silverback/application.py Show resolved Hide resolved
silverback/runner.py Outdated Show resolved Hide resolved
silverback/application.py Outdated Show resolved Hide resolved
silverback/application.py Outdated Show resolved Hide resolved
@fubuloubu

This comment was marked as outdated.

@fubuloubu fubuloubu force-pushed the feat/add-app-state branch 2 times, most recently from 20ee2b1 to 0af18b8 Compare May 30, 2024 23:47
@fubuloubu fubuloubu changed the title feat!: add app.state and parameters API feat!: add app.state May 30, 2024
@fubuloubu fubuloubu marked this pull request as ready for review May 30, 2024 23:55
@fubuloubu
Copy link
Member Author

So given that distributed TaskIQ sequencing currently operates as multi-threading (and not multi-processing), this approach of worker-managed application state is okay as long as there isn't multiple workers trying to write the same state at the same time. We may need to integrate a lock to the AppState datatype in order to ensure this, but in terms of the mechanism it will actually function as long as there is only one container with one process operating the multi-threaded workers.

@fubuloubu fubuloubu requested a review from mikeshultz May 30, 2024 23:58
mikeshultz
mikeshultz previously approved these changes May 31, 2024
example.py Outdated Show resolved Hide resolved
example.py Outdated Show resolved Hide resolved
@fubuloubu
Copy link
Member Author

fubuloubu commented May 31, 2024

TODO:

  • write documentation about app.state and how it gets used

mikeshultz
mikeshultz previously approved these changes Aug 16, 2024
Copy link
Contributor

@mikeshultz mikeshultz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't really understand why the datastore is on the worker side. I'm going to have to basically re-invent that whole thing for the platform side, more or less reverting or duplicating some of this design. But I think a bunch of this was already touched in previous discussions so probably pointless to mention.

Will try and figure out how to make this work on the cluster side when I get a chance.

@fubuloubu
Copy link
Member Author

fubuloubu commented Aug 17, 2024

I still don't really understand why the datastore is on the worker side.

Originally I don't think I actually understood what you were saying, but yes it is very clear to me now that needs to be inverted. The application state being worker side is what I wanted here, but the state snapshot loading and backup should be entirely on the runner client to perform. My mistake.

I hope you agree the solution should be to update this system function and add startup_state as a parameter it receives from the runner to update the local state on startup, then update this system function to just return the state snapshot back to the runner to write to disk; lastly, can move the AppDatastore back to the SDK's BaseRunner (and maybe make that a configurable ABC if you need to use it downstream, up to you).

@fubuloubu
Copy link
Member Author

I still don't really understand why the datastore is on the worker side. I'm going to have to basically re-invent that whole thing for the platform side, more or less reverting or duplicating some of this design. But I think a bunch of this was already touched in previous discussions so probably pointless to mention.

Will try and figure out how to make this work on the cluster side when I get a chance.

@mikeshultz resolved in 07dd9f9

johnson2427
johnson2427 previously approved these changes Aug 26, 2024
mikeshultz
mikeshultz previously approved these changes Aug 26, 2024
Copy link
Contributor

@mikeshultz mikeshultz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question on design inline. I'm good with it as is though.

silverback/application.py Show resolved Hide resolved
johnson2427
johnson2427 previously approved these changes Oct 8, 2024
Dockerfile Show resolved Hide resolved
@fubuloubu
Copy link
Member Author

Note for reviewers: SILVERBACK_APP_NAME changes to SILVERBACK_BOT_NAME from this PR, which also made some other larger changes for the whole app -> bot rewording

Also need to publicize this, since SilverbackApp also changes to SilverbackBot

docs/userguides/development.md Outdated Show resolved Hide resolved
docs/userguides/development.md Outdated Show resolved Hide resolved
docs/userguides/development.md Outdated Show resolved Hide resolved
docs/userguides/development.md Outdated Show resolved Hide resolved
docs/userguides/development.md Outdated Show resolved Hide resolved
docs/userguides/development.md Outdated Show resolved Hide resolved
docs/userguides/development.md Outdated Show resolved Hide resolved
docs/userguides/development.md Outdated Show resolved Hide resolved
docs/userguides/development.md Outdated Show resolved Hide resolved
docs/userguides/development.md Outdated Show resolved Hide resolved
@johnson2427 johnson2427 merged commit 7bd78a3 into main Oct 9, 2024
22 checks passed
@johnson2427 johnson2427 deleted the feat/add-app-state branch October 9, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants