Remove periodic data transfer#408
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the time-based (periodic) data transfer mechanism from the server, leaving only event-driven transfer based on batch size (plus explicit transfer actions), in order to improve reliability and avoid overlapping background work with other storage operations.
Changes:
- Remove
transfer_time_in_secondsfrom CLI/configuration, server configuration management, and remote UpdateConfiguration handling. - Remove the periodic transfer scheduler task from the data transfer component and storage engine initialization.
- Update documentation/tests to reflect the removed setting.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/user/README.md | Removes the documented --transfer-time-in-seconds setting from the configuration table. |
| crates/modelardb_types/src/flight/protocol.proto | Removes transfer_time_in_seconds from the public Flight protobuf surface (but currently renumbers fields/enum values). |
| crates/modelardb_server/tests/integration_test.rs | Removes assertions/tests related to the removed transfer-time setting. |
| crates/modelardb_server/src/storage/mod.rs | Stops starting/maintaining the periodic transfer scheduler via the storage engine. |
| crates/modelardb_server/src/storage/data_transfer.rs | Removes the scheduler handle and periodic transfer logic from DataTransfer. |
| crates/modelardb_server/src/remote/mod.rs | Removes UpdateConfiguration handling for the removed transfer-time setting. |
| crates/modelardb_server/src/main.rs | Removes the transfer-time CLI argument and updates the batch-size argument docs. |
| crates/modelardb_server/src/configuration.rs | Removes transfer-time from persisted configuration and live update logic; updates batch-size docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
skejserjensen
approved these changes
Jul 6, 2026
chrthomsen
approved these changes
Jul 6, 2026
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 removes periodic data transfer due to reliability and consistency issues. Event-driven data transfer based on the batch size is still available.
The periodic data transfer is removed since it was the only part of the system where we had a time-based background task (or spawned background task in general) instead of event-driven handling. This change makes data transfer consistent with the rest of the system.
Furthermore, reliability issues was found due to potential concurrency problems. A background task running data transfer could overlap with optimize/vacuum/truncate and cause issues. This becomes an even larger problem when an automatic (event-driven) data optimizer is added.
When reviewing the PR, also consider whether these reasons are enough to justify the removal.