-
Notifications
You must be signed in to change notification settings - Fork 666
Directory structure impl #1879
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
Directory structure impl #1879
Conversation
77aa734 to
4979700
Compare
89bf1b1 to
3faa3ef
Compare
3faa3ef to
5be1902
Compare
|
Some other nits:
|
cb25739 to
598faa2
Compare
gefjon
left a comment
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.
Many thanks for the in-depth PR description. That makes review much easier.
jsdt
left a comment
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.
This seems like an improvement to me, though it would be a good time to add some more comments. I also wrote a few unit tests that you could add: noa/directory-structure...jsdt/lock-test
bfops
left a comment
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.
CLI changes LGTM
badf7d6 to
7bcacb3
Compare
7bcacb3 to
20849f8
Compare
20849f8 to
e501984
Compare
Description of Changes
Implements the new directory structure proposal, using strongly-typed newtypes to represent paths. All places in the database/server/cli that previously would have been passing around
PathBufs and.join()ing them now instead pass around these types, calling methods on them that internally call.join()and return another strongly-typed child node in the directory hierarchy.Changes from the proposal implemented here:
replicas/$database_identity/$replica_id/database/..., to justreplicas/$replica_id/...SPACETIMEDB_DISABLE_DISK_LOGGING,SPACETIMEDB_TRACY,SPACETIMEDB_FLAMEGRAPH{,_PATH}) that seem to be just for debugging purposes. I can turn them into command-line args if that's desired.~/.spacetimedirectory across the xdg base directories on unix-likes (so~/.config/spacetime,~/.local/share/spacetime, etc). On windows it's moved to%LocalAppData%/SpacetimeDBconfig.tomlintocli.tomland data-dirconfig.toml, and implementeddata-dir/metadata.toml.data-dir/wasmtime_cache->data-dir/cache/wasmtime0.log(though a proper logrolling mechanism hasn't been implemented - that likely needs further work), database logs are indata-dir/logsinstead of/var/logs/spacetime.data-dir/spacetime.pidlockfile, so multiple processes can't use the same data-dir at the same time.cli-{bin-file,bin-dir,config-dir},data-dir) are now bundled in theSpacetimePathstype. server processes like standalone only interact with the data-dir.spacetime --root-dirandspacetime --config-path, to set a custom root directory for the spacetime installation and to specify a customcli.toml, respectively. this was not in the proposal itself but something like this had to be implemented for our tests to still work.Changes that are not directly related to that, but facilitate it:
spacetimedb::auth::JwtKeystype, which deduplicates jwt-key-file code across our server implementations. This is something I have less good motivation for, and I could try and revert if desired.Host::try_initnow takes&HostController, instead of like 5 different args that were just fields onHostControllerpassed individually. I was adding a 6th arg and clippy was getting mad at me for having too many parameters.EngineandLinkerare now stored in aWasmtimeRuntimestruct. Since we have to giveEnginethe{data-dir}/cache/wasmtimepath, it made more sense for that to just get eagerly initialized byHostController, who has access to the data-dir path, instead of threadingdata_dirall the way through towasmtime::make_actor.relational_db::test_utils::TempReplicaDir, which merges aTempDirandspacetimedb_paths::server::ReplicaDir.spacetimedb::startup::TracingOptionsgot added - this is so we wouldn't read environment variables incoreanymore.