server: implement SqliteStore database storage backend - #35
Conversation
042da50 to
6bc9651
Compare
| commit.encode(&mut buf).unwrap(); | ||
| let _ = tokio::task::spawn_blocking(move || { | ||
| let conn = conn.lock().unwrap(); | ||
| let _ = conn.execute( |
There was a problem hiding this comment.
Quick question on put_commit, put_tree,... methods, I noticed these methods dont return a result or check if query execution was actually successful or not. Looking at the code, it feels like we're assuming the write will always succeed, but DB writes can fail.
What happens if a write fails here? In case of failure, shouldn't our flow reflect that and return an error back to the client?
There was a problem hiding this comment.
good catch, with this implementation database write errors are swallowed on put_tree, put_commit, etc which would be very problematic. The server and then the user/client should know if any database writes and also reads have failed due to a database error.
I've added another PR to add a new StoreResult type that handles this
There was a problem hiding this comment.
FYI - this is the kind of thing you want to add error-path testing for in your integration test suite. so far you've only been writing happy path tests and those are a good start but it really helps to be able to force failure conditions and make guarantees about what results.
There was a problem hiding this comment.
Added error path testing in the following PR for each error type possible (unit tests as well as CLI integration tests asserting proper error message for db being read only, for table not existing, etc)
5560e67 to
b75367c
Compare
b75367c to
f7dd556
Compare
| let _ = tokio::task::spawn_blocking(move || { | ||
| let conn = conn.lock().unwrap(); | ||
| let _ = conn.execute( | ||
| "INSERT OR REPLACE INTO files (repo_id, file_id, content, data) VALUES (?1, ?2, ?3, ?3)", |
There was a problem hiding this comment.
This does not matching the schema for files.
There was a problem hiding this comment.
fixed. Now writes (repo_id, file_id, data) matching schema
929c01b to
f65f2a0
Compare
f65f2a0 to
1c366d4
Compare
1c366d4 to
309eef5
Compare
309eef5 to
a3f6d5f
Compare
| commit_id BLOB NOT NULL, | ||
| data BLOB NOT NULL, | ||
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| PRIMARY KEY (repo_id, commit_id) |
There was a problem hiding this comment.
I think this is good.... ideaaaaallly the commit ID should actually be universally unique. So theoretically I think you could compact a little and not store the same commit twice. But doing it this way is probably fine? Same goes for most of these.
Anyway I don't think you have to change it, just interesting bit of trivia.
|
|
||
| fn init_tables(&self) -> Result<(), rusqlite::Error> { | ||
| let conn = self.conn.lock().unwrap(); | ||
| let schema = include_str!("../../db/schema_sqlite.sql"); |
There was a problem hiding this comment.
Suggest putting the path the the schema as an environment variable or a constant you can configure at runtime instead. At VERY LEAST please put it as a const with the definition at the top of this file.
There was a problem hiding this comment.
added the schema path as a constant at the top and added a comment explaining. I think we don't want the schema as an environment variable as this is part of the source code, and not something we want to be able to change on server startup.
| commit.encode(&mut buf).unwrap(); | ||
| let _ = tokio::task::spawn_blocking(move || { | ||
| let conn = conn.lock().unwrap(); | ||
| let _ = conn.execute( |
There was a problem hiding this comment.
FYI - this is the kind of thing you want to add error-path testing for in your integration test suite. so far you've only been writing happy path tests and those are a good start but it really helps to be able to force failure conditions and make guarantees about what results.
| std::process::exit(1); | ||
| let path = args.sqlite_path.unwrap_or_else(get_default_sqlite_path); | ||
| info!("Opening SQLite Database Store at '{}'", path.display()); | ||
| Arc::new(store::SqliteStore::open(path)?) |
There was a problem hiding this comment.
when trying to touch the filesystem there are a ton of things that can go wrong and I don't really see error handling happening...
what if ~/.jj-cc-server/commit_cloud.db already exists? what if I don't have permission to access ./commit_cloud.db and I don't have $HOME set? what if the disk is full? what if the disk fails in the middle of the write?
There was a problem hiding this comment.
Error handling is added for the SQLite path>
If the database path already exists, it runs the setup (CREATE TABLE IF NOT EXISTS).
If there is no permission to access the database, it throws Error: Failed to open database (this is tested in the test_sqlite_store.rs CLI integration test)
If $HOME is not set, it logs a warning, and defaults to ./commit_cloud.db.
If the database ends up not existing, any subsequent read and writes will fail with the appropriate message, as implemented and tested in the subsequent PR
d938286 to
624645e
Compare
Implement the SQLite storage backend for the server by implementing the Store trait so repository data is saved to a local SQLite database file instead of being stored in memory. Add the SQLite database schema with a schema path constant and implement storage methods for repositories, commits, trees, files, operations, and views. Run database queries on blocking threads so the async server threads are not blocked. Store the optional database path on the SqliteStore struct with a reconnect method, and add robust filesystem error handling for SQLite path resolution. Add unit tests verifying SQLite put and read operations for all entity types (files, commits, trees, operations, views, op heads, and repo registration), path getters, reconnection preserving disk data, and default SQLite path resolution happy and failure paths. Add CLI integration tests covering happy path working copy snapshots and failure paths for parent directory creation blockers, unpermitted files, and unwritable default home directories asserting exact stderr error output, and remove #[should_panic] from the SQLite integration test. Update testutils/build.rs with workspace manifest path resolution and stale binary cleanup to build jj-cc-server reliably across crates.
624645e to
dd1e531
Compare
3 participants
No description provided.