Share one local and one remote DataFolder instance throughout the server - #419
Open
CGodiksen wants to merge 13 commits into
Open
Share one local and one remote DataFolder instance throughout the server#419CGodiksen wants to merge 13 commits into
CGodiksen wants to merge 13 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #416 by switching server components to share a single local DataFolder and (when configured) a single remote DataFolder instance via Arc, reducing redundant construction of the DataFolder’s cache and DataFusion session context. It also improves Delta table cache access to avoid holding a DashMap shard lock across async I/O, and cleans up a few naming inconsistencies (e.g., “folder” vs “path/object store”).
Changes:
- Share
DataFolderinstances across the server usingArc<DataFolder>instead of cloningDataFolder. - Avoid holding a DashMap guard while awaiting
DeltaTable::load()when returning cached tables. - Rename parameters/fields in a few places to better reflect what is being passed (paths/object stores).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/modelardb_storage/src/data_folder/mod.rs | Removes Clone from DataFolder, stores a single SessionContext, and updates Delta table caching to avoid async I/O while holding a DashMap guard. |
| crates/modelardb_server/tests/integration_test.rs | Renames variables/params to reflect that a temp dir path is passed to the server CLI. |
| crates/modelardb_server/src/storage/uncompressed_data_manager.rs | Switches local_data_folder ownership to Arc<DataFolder> for shared use. |
| crates/modelardb_server/src/storage/uncompressed_data_buffer.rs | Renames “local_data_folder” object store parameters/fields to local_object_store for clarity. |
| crates/modelardb_server/src/storage/data_transfer.rs | Uses shared Arc<DataFolder> for local/remote folders; updates tests accordingly. |
| crates/modelardb_server/src/storage/data_storage_compactor.rs | Uses shared Arc<DataFolder> for the compactor’s local folder; updates tests accordingly. |
| crates/modelardb_server/src/storage/compressed_data_manager.rs | Stores the manager’s local_data_folder as Arc<DataFolder> and updates constructor/tests. |
| crates/modelardb_server/src/data_folders.rs | Makes DataFolders hold Arc<DataFolder> and updates try_from_args() to construct shared instances. |
| crates/modelardb_server/src/context.rs | Updates tests to wrap DataFolder in Arc before constructing DataFolders. |
| crates/modelardb_server/src/configuration.rs | Changes ConfigurationManager to hold Arc<DataFolder>; updates tests that construct DataFolders. |
| crates/modelardb_server/src/cluster.rs | Updates Cluster to store remote_data_folder as Arc<DataFolder> and adjusts tests/helpers to avoid cloning DataFolder. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
skejserjensen
approved these changes
Aug 13, 2026
| async fn delta_table_from_path(&self, table_path: &str) -> Result<DeltaTable> { | ||
| // Use the cache if possible and load to get the latest table data. | ||
| if let Some(mut delta_table) = self.delta_table_cache.get_mut(table_path) { | ||
| // Clone the cached table out if possible and drop the DashMap guard before loading. load() |
Contributor
There was a problem hiding this comment.
I do not understand what "... clone out ... " means.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR closes #416 by sharing a single local data folder and a single remote data folder throughout the server by wrapping them in an
Arc. While looking into if there were any problems with sharing a singleDataFolderinstance, a small locking inefficiency was found when opening aDeltaTable. This is also fixed in this PR. Finally, the PR also fixes some naming inconsistencies, specifically regarding arguments being namedlocal_data_folderwhile actually being a path or an object store.