Skip to content

Commit

Permalink
Add convenience wrapper for disk uploads to SDK (#840)
Browse files Browse the repository at this point in the history
From a user perspective uploading a disk is a single operation, but
doing so via the SDK is a very involved process, involving stringing
together multiple API calls with any interruption leaving the disk in a
state that cannot be directly removed.

Add a new `extras` module to the SDK that will contain convenience
wrappers around actions that functions that are overly complicated.
Create a `disk_import` function in `extras` that will perform a disk
upload as a single step, and attempts to cleanup partially imported
disks on failure. We make the `disk upload` CLI command a thin wrapper
around the new SDK function, responsible only for cancellation and
progress. We manually recreate the builder interface, as creating a
procmacro to do this would require creating a new crate. If/when
`extras` continues to grow we should reconsider this.

`disk_import` also give callers access to a `DiskImportHandle` with a
`watch` channel with the current number of bytes uploaded for use in
progress tracking, and provides a `cancel` function that allows users
to gracefully stop the disk upload. A disk that has been successfully
created will not be removed, only those that are partially imported.

The `cancel` function uses `tokio::select` to cancel the `Future` of
the upload task. This should be safe in our in our case no locks are
held across `await` points. No operations need to be safe to restart as
we only call `tokio::select` a single time, they will be dropped
immediately and losing a message is not a problem.

We are forced to remove tests `test_disk_import_bulk_import_start_fail`
and `test_disk_import_bulk_write_import_fail` as we now poll for disks
in `Created` state, which may happen if a user immediately cancels a new
request. This state is not eligible to be finalized, so we need to wait
for the disk to progress to the next state, but `HttpMock` does not
provide a way to programatically delete or update a mock after sending
`n` responses. This means that the disk will always be returned as
non-existing when querying, and cleanup will be skipped. I have opened
alexliesenfeld/httpmock#108 to add a
`delete_after_n` method, but this seems unlikely to be merged. We may
want to consider moving to `wiremock` in the long term, which provides
this functionality.

Co-authored-by: Adam Leventhal <[email protected]>
  • Loading branch information
wfchandler and ahl authored Sep 23, 2024
1 parent b90c10c commit a2bbd78
Show file tree
Hide file tree
Showing 7 changed files with 986 additions and 573 deletions.
2 changes: 1 addition & 1 deletion cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ log = { workspace = true }
md5 = { workspace = true }
oauth2 = { workspace = true }
open = { workspace = true }
oxide = { workspace = true }
oxide = { workspace = true, features = ["clap"] }
oxnet = { workspace = true }
predicates = { workspace = true }
ratatui = { workspace = true }
Expand Down
Loading

0 comments on commit a2bbd78

Please sign in to comment.