Add RetrieveFrom, to read a file from an offset - #81
Open
christhomas wants to merge 1 commit into
Open
Conversation
Requested in secsy#47. The plumbing was already there: Retrieve uses REST to resume a transfer that failed part way, so transferFromOffset already does the work. This exposes it. Retrieve is now RetrieveFrom starting at zero, which is covered by a test asserting its behaviour is unchanged. The maintainer suggested inferring the offset from dest when it is an io.Seeker, or an opt-in wrapper type. This is the explicit method the issue asked for, because a Retrieve that silently starts somewhere other than the beginning depending on the concrete type of its argument is harder to reason about than a second method — and the wrapper type is a third concept to learn for something a parameter says plainly. Four cases are decided rather than left to the server: An offset equal to the file's size copies nothing and is not an error. That is where a caller resuming an already-complete transfer lands, and failing them would be unhelpful. It also avoids asking a server for zero bytes, which invites a server-specific answer to a question with an obvious one. An offset past the end is an error. The server's own reply varies and some will happily send nothing, which is indistinguishable from success. A negative offset is an error, and never reaches the wire as "REST -1". An offset above zero against a server without REST STREAM is an error. Retrieve merely loses the ability to retry in that case; RetrieveFrom cannot start at all, so it says so rather than quietly sending the whole file from the beginning.
Open
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.
Implements the request in #47.
The plumbing was already there:
Retrieveuses REST to resume a transfer that failed part way, sotransferFromOffsetalready does this work. This exposes it.RetrievebecomesRetrieveFrom(path, dest, 0), with a test asserting its behaviour is unchanged.On the API shape
You suggested:
I went with the explicit method @gauravtiwari-tw asked for. A
Retrievethat starts somewhere other than the beginning depending on the concrete type of its argument is harder to reason about than a second method — the caller has to know that*os.Filebehaves differently from abytes.Buffer— and a wrapper type is a third concept to learn for something a parameter says plainly.Happy to swap it for either of yours if you'd rather; the underlying change is one line either way.
Four cases decided here rather than left to the server
== size> size< 0REST -1> 0withoutREST STREAMRetrievemerely loses the ability to retry; this cannot start at all, so it says so rather than quietly sending the whole file from the beginningOne note on running the tests: this branch is off
master, soTestMainstill requires./build_test_server.sh. I verified via the container harness in #73.