feat: Upload concurrency and backpressure control for shared cache #584
Conversation
flub
commented
Nov 26, 2021
•
flub
commented
- Fixup tests
- Atomic writes for filesystem
- Move concurrency controls to config
- Figure out sane defaults
Because store is not async and only sends a message the original caller has no idea if it ever finishes. This adds a oneshot channel to send back a completion message. It makes testing possible. This also changes the tests to not return Result anymore, the errors are still useless with them. With .unwrap() the test failures at least point to where the error came from.
We should be testing that we're not uploading to GCS twice.
Plus give them sensible defaults for our prod. Config examples in the tests.
|
PTAL, updated the config to allow setting concurrency control and queue size. Defaults based on some guesstimating of our prod infrastructure. |
| let temp_file = NamedTempFile::new_in(&temp_dir)?; | ||
| let dup_file = temp_file.reopen()?; |
There was a problem hiding this comment.
i see that this pattern was also used in the original shared cache PR. what is the purpose of this? is it to convert NamedTempFile into a File?
There was a problem hiding this comment.
In order to get a tokio::fs::File without re-opening from the path (which is unsafe) we need to get a duplicate filedescriptor which points to the same file. tokio::fs::File consumes the std::fs::File so the reference returned by NamedTempFile::as_file() is not sufficient and we actually need to dup the filedescriptor, the nicest way is to do NamedTempFile::reopen (it is also possible from std::fs::File::try_clone on the &File)
Co-authored-by: relaxolotl <5597345+relaxolotl@users.noreply.github.com>
