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

[SIP] sqlite host component #1408

Merged
merged 16 commits into from
May 22, 2023
Merged

[SIP] sqlite host component #1408

merged 16 commits into from
May 22, 2023

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented Apr 18, 2023

This introduces a SIP for adding a sqlite interface to Spin as well as an initial MVP implementation.

Much like the key-value interface, this would be a "batteries included" implementation where users gain access to a SQL database with no configuration required.

I have also created an example TODO application that uses this MVP implementation.

docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
@rylev rylev force-pushed the sqlite-host-component branch 3 times, most recently from 2fa06b7 to f59a9ed Compare April 20, 2023 17:44
@rylev rylev requested a review from itowlson April 20, 2023 17:45
@rylev
Copy link
Collaborator Author

rylev commented Apr 20, 2023

@itowlson I think this is ready for another look. The implementation pretty much matches the SIP now. That means the open questions still remain, but we have a working prototype. I guess the question now is: should we answer the open questions or merge and then figure out the open questions?

(FYI: the CI failure is because spin-componentize needs to be updated)

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Commented only on the SIP (haven't read through the implementation but can if you want)

docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
crates/sqlite/src/lib.rs Outdated Show resolved Hide resolved
crates/sqlite/src/lib.rs Outdated Show resolved Hide resolved
crates/sqlite/src/lib.rs Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
@rylev rylev marked this pull request as draft April 27, 2023 16:08
@michelleN michelleN added this to the 1.2.0 milestone May 1, 2023
@michelleN michelleN removed this from the 1.2.0 milestone May 2, 2023
Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

Quick proofreading review for the SIP

docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/000-sqlite.md Outdated Show resolved Hide resolved
@rylev rylev force-pushed the sqlite-host-component branch 4 times, most recently from 616fc5c to 7d97005 Compare May 10, 2023 10:27
@rylev
Copy link
Collaborator Author

rylev commented May 10, 2023

I am marking this PR as ready to review. While this feature is not complete, I do believe it is useful enough for folks to start experimenting with. Below I've tried to capture the current state and the open questions we need to answer before we can officially make this a stable feature:

This PR provides the following features:

  • Support in spin for a sqlite host implementation that provides a default sqlite database when running spin locally.
  • Support for runtime configuration for adding additional local sqlite databases.
  • Rust SDK support for targeting these databases from a Rust based spin component
  • The ability to run arbitrary sql statements against the default database on start up with spin up --sqlite-migration="YOUR SQL HERE"

There are still some open questions and things to discuss/think about:

  • I do not believe this feature is ready to marked stable. As such I have put sqlite support behind a feature flag in the only SDK that currently supports it: Rust. This will still allow folks to target sqlite if they create spin components manually (i.e., using wit-bindgen directly), but I believe we already consider such usage experimental and not officially supported by our backwards compatibility guarantees. If we believe that by merging this PR that we are committing to backwards compatibility of this feature in its current form, then I think we should wait and consider how we want to allow for experimentation before committing to backwards compatibility.
  • There are still some open questions in the SIP with regards to sqlite versioning and capacity limits. I don't believe those need to be solved before experimentation can begin.
  • There is no support for this feature in Fermyon Cloud. Since this is an experimental feature only, I believe that this is fine for now - though we will quickly want to change that.

@rylev rylev marked this pull request as ready for review May 10, 2023 10:52
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Some comments / questions / nits but nothing that can't be iterated on as we build it out. Nice one - thank you!

crates/loader/src/bindle/config.rs Outdated Show resolved Hide resolved
docs/content/sips/013-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/013-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/013-sqlite.md Outdated Show resolved Hide resolved
docs/content/sips/013-sqlite.md Outdated Show resolved Hide resolved
sdk/rust/src/lib.rs Outdated Show resolved Hide resolved
@itowlson
Copy link
Contributor

By the way, what is the order of landing things for the first iteration? I saw the PR took a dependency on a branch of spin-componentize. Is the idea that we would land spin-componentize first? Is that going to be the general workflow for as long as we need to continue to support modules? (cc @dicej)

sdk/rust/src/sqlite.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dicej dicej left a comment

Choose a reason for hiding this comment

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

Looks great overall.

The async { $BODY }.await stuff is aggravating my OCD, mainly just because $BODY does synchronous I/O and no async, so I'm definitely in favor of using block_in_place instead. No need to split each method, either -- just make it task::block_in_place(|| { $BODY }).

crates/trigger/src/runtime_config.rs Show resolved Hide resolved
crates/sqlite/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

LGTM, and the todo example works great for me!

This is really exciting, thanks for working on it, @rylev!

@rylev
Copy link
Collaborator Author

rylev commented May 12, 2023

I think this is ready to merge. We'll wait until the next version of spin is released so that we have a full release cycle to let this bake.

#### Interface open questions

**TODO**: answer these questions
* `row-result` can be very large. Should we provide some paging mechanism or a different API that allows for reading subsets of the returned data?
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could hedge on this by changing the function name to e.g. query-all, reserving query for a future cursor API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've addressed this in the latest commit.

rylev added 15 commits May 22, 2023 13:18
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev merged commit 276b5cf into main May 22, 2023
@rylev rylev deleted the sqlite-host-component branch May 22, 2023 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants