SQL-3225: seperate cli and schema management logic from library - #148
Conversation
This commit switches the `DataService` from using `async-trait` to using the Rust-maintained `trait-variant`. This fixes an issue with using the `DataService` trait inside of `tokio::spawn` due to an old Rust compiler bug around lifetime elision in generic associated types (GAT). See [1] for more info and [2] for a response from the Rust team about the `trait-variant` crate and why it was added. [1]: rust-lang/rust#100013 [2]: https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits/#async-fn-in-public-traits
This alias was confusing since it violated the principle of least surprise by looking like a std::result::Result.
| use collection::{DatabaseCollections, query_for_initial_schemas}; | ||
| mod partitioning; | ||
| use partitioning::get_partitions; | ||
| pub use partitioning::{PartitionedCollection, get_partitions}; |
There was a problem hiding this comment.
note: pub exports, the core change of this pr.
| #[cfg_attr(feature = "wasm", async_trait::async_trait(?Send))] | ||
| pub trait DataService { | ||
| #[trait_variant::make(DataService: Send)] | ||
| pub trait LocalDataService { |
There was a problem hiding this comment.
Just wanted to point out the the switch from async_trait to trait_variant was necessitated by a long-standing rust bug (found here) causing issues when using this trait with tokio. When async in traits was stabilized, the Rust blog brought up this exact problem and solution, as shown here.
This new procedural macro does come with the added benefit that downstream consumers can decide whether or not they care about the added Send bounds by using either LocalDataService or DataService, which is shown in the changes made to the mongodb and wasm backed clients.
nicholascioli
left a comment
nicholascioli
left a comment
There was a problem hiding this comment.
Just left a few nits, but LGTM. I can't approve the PR since I wrote some of it, but thanks for putting this together!
| @@ -26,100 +22,15 @@ pub struct SinglePartition { | |||
|
|
|||
| pub const PARTITION_DOCS_PER_ITERATION: i64 = 20; | |||
There was a problem hiding this comment.
Nit: This probably doesn't need to be pub anymore.
| ignored = [ | ||
| "wasm-bindgen-futures" # Note, this should be removed when the following is resolved: https://github.com/wasm-bindgen/wasm-bindgen/issues/5104#issuecomment-4223185700 | ||
| ] No newline at end of file | ||
| ] |
There was a problem hiding this comment.
Nit: We might want to add the newline at the end here
There was a problem hiding this comment.
Praise: I love to see dependencies get removed
NoIdInSample, InitialSchemaError, and ChannelClosed were emitted only from code paths that were removed when trimming the library to leaf primitives. None of the remaining schema-builder-library code emits them, and downstream consumers (schema-builder-orchestration) carry their own Error enum, so removing them from the library's public Error enum is safe.
bucaojit
left a comment
bucaojit
left a comment
There was a problem hiding this comment.
The changes look good and I like the idea of separation of concerns. That does clean things up and will help your upcoming work.
4 participants
During the process of creating WASM vs Multi-threaded schema manager, we discovered that it was difficult to support different forms of orchestration without requiring the duplication of code. Namely, we could easily do things like getting partitions, deriving schema for a partition, etc atomically, but for broader, parallelizable tasks like creating tasks for each collection to construct a schema, we started needing multiple code paths.
After some discussion, it was determined a cleaner, more sustainable approach would be to focus on "separation of concerns" -- that is, we will have core schema generation logic in the schema-builder-library, while deferring orchestration (how do we get schemas for every collection in a database? how do we get schemas for every collection in a cluster? and of course, creating a cli to manage this) to downstream consumers. A mitigation for this for other downstream consumers (wasm) is that we can add higher level, synchronous functions for deriving a schema for a collection in the bindings.
This PR removes the bulk of the logic that is more of a "schema manager" concern, while making certain functions public for the schema manager to pull in. The companion pr on schema manager can be found here. The major followup to this will be SQL-3167, which will cover adding anything needed to the wasm bindings and ensuring it still compiles.
There are likely ergonomic benefits to be gained for callers of the library that are not wasm or schema manager; that is, things we could do to make using this api easier. Some ideas:
tokio::spawnfrom schema manager and move that function herethe api as it stands is entirely bare bones, enough for schema manager, wasm, and an internal user (with our guidance). The above proposals (which can go in the followup schema manager improvements epic) would help users to not want/need our guidance to implement