Fix roll_time and auto_commit settings for FileSink
#859
Conversation
This file contains 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
This hopefully clears up, at least for the use of the FileSinkWriteExt trait the relationship between `auto_commit` and `roll_time`. There is no default implementaion for this option, as all options have implications that should be considered.
The PR that updated FileSink to use the Ext trait had an incorrect understanding of the relationship between `roll_time` and `auto_commit`. That understanding has since been corrected and codified in the `FileSinkCommitStrategy` enum. For this commit, https://github.com/helium/oracles/pull/849/files was combed through to get all correct settings for FileSinks before they were transitioned to use FileSinkWriteExt::file_sink().
michaeldjeffrey
force-pushed
the
mj/roll-time-auto-commit
branch
from
f96a2cd
to
07ee3d4
Compare
jeffgrunewald
approved these changes
Aug 30, 2024
andymck
reviewed
Aug 30, 2024
andymck
approved these changes
Aug 30, 2024
* Change so auto_commit and roll_time are separated * Create FileSinkRollTime enum
bbalser
approved these changes
Aug 30, 2024
Yes! 🎸 |
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.
In a previous update to
file_storeI had a misunderstanding of theauto_commitandroll_timesettings. I thoughtauto_commit=truewould cause a file sink to commit itself on every write, which would mean the settings were in opposition of each other.Then arose such a clatter (Oh the folly of my ways)
File Sinks where the settings had previously set
auto_commit=trueandroll_time=any_durationwere converted to only theroll_timebeing set andauto_commit=false.These file sinks were expecting
max_sizeorroll_timeto be the driver of when a file was uploaded to s3. Withauto_commit=false, the file sink was rolling the file into temporary storage, waiting for a.commit()that would never come.Add
FileSinkCommitStrategyandFileSInkRollTimeto clear up relationship betweenroll_timeandauto_commit.Having to make both of these decisions when creating a file sink decreases the likelihood of building up a stash of tmp files that are not being uploaded because a Manual sink was created when an Automatic sink was desired.
I went through https://github.com/helium/oracles/pull/849/files to get grab the settings before they transitioned to use
FileSinkWriteExt::file_sink().For anyone spot checking against that PR, the defaults are.
roll_time: Duration::from_secs(DEFAULT_SINK_ROLL_SECS)(3 minutes)auto_commit: true