Replace time based logic with atomic rename for write#2897
Conversation
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. |
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.
breedx-splk
left a comment
There was a problem hiding this comment.
I haven't fully reviewed this yet, but I wanted to ask the same question I asked in the issue: What happens to the staging files when the process exits and restarts? Will the staged files be moved over or ?
Ok, I see it now, it does look like there is explicit code to cover this case and recover the "orphaned" files. Killer! Thanks. |
Apologies, I missed the original question! Happy to see you liked the approach :) |
|
Hello @breedx-splk is anything blocking this PR? Apologies for the ping, just checking! |
|
I think @breedx-splk is OOO for a while. Can anyone else take a look? Or should we wait until @breedx-splk is back? @open-telemetry/java-contrib-approvers |
|
I'm back and taking a look now, sorry for the lag. Were all of the copilot comments responded to with code? If so, can you please close those? |
|
Looks good to me. I definitely favor this approach over the pervious timeout-based one. Not yet giving approval, because I want to ensure that the other comments are all addressed (including some one that @LikeTheSalad pointed out). Thanks for this contribution! 🍻 |
|
Hi @breedx-splk thanks for the feedback! |
|
@LikeTheSalad ready for merge? I'm into it. Thanks again @mamazzol for the contribution! |
I'm ready, thanks! |
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: