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

Directory structure impl #1879

Merged
merged 9 commits into from
Nov 12, 2024
Merged

Directory structure impl #1879

merged 9 commits into from
Nov 12, 2024

Conversation

coolreader18
Copy link
Collaborator

@coolreader18 coolreader18 commented Oct 18, 2024

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:

  • the replicas directory in the data-dir is flattened from having database files be at replicas/$database_identity/$replica_id/database/..., to just replicas/$replica_id/...
  • all environment variables are removed, besides a few (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.
  • I split the ~/.spacetime directory across the xdg base directories on unix-likes (so ~/.config/spacetime, ~/.local/share/spacetime, etc). On windows it's moved to %LocalAppData%/SpacetimeDB
  • Split the config.toml into cli.toml and data-dir config.toml, and implemented data-dir/metadata.toml.
  • data-dir/wasmtime_cache -> data-dir/cache/wasmtime
  • module logs are dated instead of just 0.log (though a proper logrolling mechanism hasn't been implemented - that likely needs further work), database logs are in data-dir/logs instead of /var/logs/spacetime.
  • data-dir/spacetime.pid lockfile, so multiple processes can't use the same data-dir at the same time.
  • All the root paths (cli-{bin-file,bin-dir,config-dir}, data-dir) are now bundled in the SpacetimePaths type. server processes like standalone only interact with the data-dir.
  • the CLI has 2 new flags for the base command itself, not on subcommands - spacetime --root-dir and spacetime --config-path, to set a custom root directory for the spacetime installation and to specify a custom cli.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:

  • the spacetimedb::auth::JwtKeys type, 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_init now takes &HostController, instead of like 5 different args that were just fields on HostController passed individually. I was adding a 6th arg and clippy was getting mad at me for having too many parameters.
  • the wasmtime Engine and Linker are now stored in a WasmtimeRuntime struct. Since we have to give Engine the {data-dir}/cache/wasmtime path, it made more sense for that to just get eagerly initialized by HostController, who has access to the data-dir path, instead of threading data_dir all the way through to wasmtime::make_actor.
  • relational_db::test_utils::TempReplicaDir, which merges a TempDir and spacetimedb_paths::server::ReplicaDir.
  • Added spacetimedb::startup::TracingOptions got added - this is so we wouldn't read environment variables in core anymore.

@coolreader18 coolreader18 force-pushed the noa/directory-structure branch 4 times, most recently from 77aa734 to 4979700 Compare October 21, 2024 05:31
@coolreader18 coolreader18 force-pushed the noa/directory-structure branch 16 times, most recently from 89bf1b1 to 3faa3ef Compare October 23, 2024 17:02
@coolreader18 coolreader18 force-pushed the noa/directory-structure branch from 3faa3ef to 5be1902 Compare October 23, 2024 21:51
@mamcx
Copy link
Contributor

mamcx commented Oct 24, 2024

Some other nits:

  • It will be nice to see a test like in my pr
  • Also, to add in the lib a copy of the proposal some the reviewers and future readers see the big picture.
  • In general, no code outside the Paths should build the layout, names, etc.

@coolreader18 coolreader18 force-pushed the noa/directory-structure branch from cb25739 to 598faa2 Compare October 28, 2024 18:52
Copy link
Contributor

@gefjon gefjon left a 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.

Copy link
Contributor

@jsdt jsdt left a 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

Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

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

CLI changes LGTM

@coolreader18 coolreader18 force-pushed the noa/directory-structure branch 5 times, most recently from badf7d6 to 7bcacb3 Compare November 4, 2024 18:00
@jdetter jdetter removed their request for review November 8, 2024 18:10
@coolreader18 coolreader18 force-pushed the noa/directory-structure branch from 7bcacb3 to 20849f8 Compare November 8, 2024 18:50
@coolreader18 coolreader18 force-pushed the noa/directory-structure branch from 20849f8 to e501984 Compare November 11, 2024 23:48
@coolreader18 coolreader18 added this pull request to the merge queue Nov 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2024
@coolreader18 coolreader18 added this pull request to the merge queue Nov 12, 2024
Merged via the queue into master with commit f136670 Nov 12, 2024
7 of 8 checks passed
@coolreader18 coolreader18 deleted the noa/directory-structure branch November 12, 2024 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break A PR that makes an ABI breaking change release-rc1-nice-to-have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants