Replace time based logic with atomci rename for write#2897
Open
mamazzol wants to merge 1 commit into
Open
Conversation
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR changes disk-buffering writer/reader coordination to use a “stage then rename-on-close” workflow, reducing reliance on time-based read delays and improving crash recovery for in-flight writes.
Changes:
- Writer now appends to
<timestamp>.tmpand promotes to the final numeric filename on close. - Folder startup recovers orphan
*.tmpfiles by promoting them to final names when safe. - Configuration/docs updated to make
minFileAgeForReadMillisoptional (default0) and update tests accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/internal/storage/files/WritableFile.java | Implements staging + rename-on-close promotion behavior. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/internal/storage/FolderManager.java | Recovers orphan temp files and allows reader to close expired writer to surface data. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/storage/impl/FileStorageConfiguration.java | Updates defaults/docs and removes min-read-age vs max-write-age validation. |
| disk-buffering/src/test/java/io/opentelemetry/contrib/disk/buffering/internal/storage/files/WritableFileTest.java | Adds/updates tests for staged writes and promotion-on-close behavior. |
| disk-buffering/src/test/java/io/opentelemetry/contrib/disk/buffering/internal/storage/StorageTest.java | Updates expectations to account for visible .tmp files while writing. |
| disk-buffering/src/test/java/io/opentelemetry/contrib/disk/buffering/internal/storage/FolderManagerTest.java | Adds coverage for orphan *.tmp recovery and adjusts filename expectations. |
| disk-buffering/README.md | Updates user-facing explanation for rename-on-close synchronization. |
| disk-buffering/DESIGN.md | Documents the new synchronization approach and crash recovery. |
| CHANGELOG.md | Notes behavioral/config changes for the release notes. |
Comment on lines
93
to
100
| public synchronized void close() throws IOException { | ||
| if (isClosed.compareAndSet(false, true)) { | ||
| out.close(); | ||
| if (!staging.renameTo(destination)) { | ||
| throw new IOException("Could not rename " + staging + " to " + destination); | ||
| } | ||
| } | ||
| } |
Comment on lines
93
to
100
| public synchronized void close() throws IOException { | ||
| if (isClosed.compareAndSet(false, true)) { | ||
| out.close(); | ||
| if (!staging.renameTo(destination)) { | ||
| throw new IOException("Could not rename " + staging + " to " + destination); | ||
| } | ||
| } | ||
| } |
Comment on lines
+58
to
85
| private void recoverOrphanTempFiles() { | ||
| File[] existingFiles = folder.listFiles(); | ||
| if (existingFiles == null) { | ||
| return; | ||
| } | ||
| for (File file : existingFiles) { | ||
| String name = file.getName(); | ||
| if (!name.endsWith(STAGING_SUFFIX)) { | ||
| continue; | ||
| } | ||
| File target = new File(folder, name.substring(0, name.length() - STAGING_SUFFIX.length())); | ||
| if (!NUMBER_PATTERN.matcher(target.getName()).matches()) { | ||
| continue; | ||
| } | ||
| if (target.exists()) { | ||
| if (!file.delete()) { | ||
| logger.warning("Could not delete duplicate orphan temp file: '" + file.getName() + "'"); | ||
| } | ||
| continue; | ||
| } | ||
| if (file.renameTo(target)) { | ||
| logger.fine( | ||
| "Recovered orphan temp file: '" + file.getName() + "' -> '" + target.getName() + "'"); | ||
| } else { | ||
| logger.warning("Could not promote orphan temp file: '" + file.getName() + "'"); | ||
| } | ||
| } | ||
| } |
Comment on lines
+124
to
+127
| /* | ||
| * If the current writable file has expired, close it and return true. | ||
| * This allows to have a readeable file without waiting for the next write to trigger the check. | ||
| */ |
Comment on lines
86
to
88
| public final FileStorageConfiguration build() { | ||
| FileStorageConfiguration configuration = autoBuild(); | ||
| if (configuration.getMinFileAgeForReadMillis() | ||
| <= configuration.getMaxFileAgeForWriteMillis()) { | ||
| throw new IllegalArgumentException( | ||
| "The configured max file age for writing must be lower than the configured min file age for reading"); | ||
| } | ||
| return configuration; | ||
| return autoBuild(); | ||
| } |
Comment on lines
+184
to
+188
| The writing and reading processes can run in parallel because they target disjoint files: the | ||
| writer appends to `<timestamp>.tmp` and only files whose name is *entirely* numeric are visible to | ||
| the reader (the reader applies `Matcher.matches()` on `\d+`, so the trailing `.tmp` excludes | ||
| in-flight files). Each rollover atomically renames the temp file to its final name via | ||
| `rename(2)`, so the reader can never observe a partially-written file. If the reader is invoked |
LikeTheSalad
reviewed
Jun 2, 2026
Contributor
LikeTheSalad
left a comment
LikeTheSalad
left a comment
There was a problem hiding this comment.
Thank you, @mamazzol 🙏
Please take a look at Copilot's suggestions because I think those are valid points. But apart from those, I think it's good to go.
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.
Description:
Feature addition: This PR replaces the time-based separation with an atomic rename
As a consequence,
minFileAgeForReadMillisis no longer required for correctness (default is now 0) and the build-time validation minFileAgeForRead > maxFileAgeForWrite has been dropped.Existing Issue(s):
#2889
Testing:
./gradlew :disk-buffering:checkis green.Documentation: