feat(storage/transfermanager): prototype #10045

BrennaEpp · 2024-04-25T17:13:38Z

No description provided.

sketch of an interface for downloads

tritone

A few initial comments, overall looks like a good start

tritone · 2024-04-25T20:10:20Z

storage/transfermanager/downloader.go

+type Downloader struct {
+	client  *storage.Client
+	config  *transferManagerConfig
+	work    chan *DownloadObjectInput // Piece of work to be executed.


Presumably this should be send-only and output should be receive?

We are sending and receiving from both channels in different places in the downloader. Unidirectional channels could be used in subcomponents or if we were providing the channel to the user, but I don't see how we could implement this with unidirectional channels - if we only received from output, who would send us the output (and vice-versa for work)?

storage/transfermanager/downloader.go

tritone · 2024-04-25T20:23:57Z

storage/transfermanager/integration_test.go

+	return crc32cHash.Sum32(), w.Close()
+}
+
+type testWriter struct {


doesn't have to be in this PR, but we should add a version of this (well, some kind of DownloaderBuffer that implements WriterAt) to the library.

Yeah, I'd have to look more into that. This is a very barebones implementation that is likely not at all efficient (and doesn't really work as a WriterAt yet).

Yeah I think that's fine for now.

storage/transfermanager/option.go

tritone · 2024-04-25T20:25:46Z

storage/transfermanager/downloader.go

+	}
+
+	// Start workers in background.
+	for i := 0; i < d.config.numWorkers; i++ {


Presumably we could optimize this by spinning up workers as needed when there are objects enqueued? Doesn't have to be in this PR though.

Sure, though I'm not sure how much that would optimize this by... I guess it depends on the num of workers.

Yeah something we can test out later.

tritone

Looks really good overall. We can chat tomorrow about a few of the remaining sync/async API changes perhaps?

tritone · 2024-05-15T23:37:09Z

storage/transfermanager/doc.go

+
+# Example usage
+
+	// Pass in any client opts or set retry policy here.


Let's add something like https://github.com/googleapis/google-cloud-go/blob/main/storage/example_test.go instead perhaps so it can be in compiled code?

tritone · 2024-05-15T23:53:16Z

storage/transfermanager/downloader.go

+// result.
+// The results of downloads initiated with this method will not be provided in
+// the results given in Downloader.Results.
+func (d *Downloader) DownloadObjectWithCallback(ctx context.Context, input *DownloadObjectInput, callback func(*DownloadOutput)) {


What is the expected behavior here if someone calls this without using the Callback option when creating a downloader? Or vice versa with DownloadObject? There are some corner cases to think through here.

I'm leaning towards just having one entry point function for DownloadObject and moving the callback to a field on the DownloadObjectInput -- what do you think?

I forgot to remove the callback Option - right now it's completely unused. We could enforce it's usage (erroring in the output or here if the option does not match the call); as of now callbacks and no callbacks could be used in the same downloader (and it should work - though it's untested).

I lean towards the two separate entry points. I think the distinction in behaviour is big enough that we should make a similar distinction in our surface - I feel like having it just as a field isn't as nice an experience for people using it and encourages more mixing of using both callback and no callbacks. If we do add it as a field, I'd suggest then also adding the output to the results returned by Results(), in addition to calling the callback if available. That way the callback is more of an optional field that would get called and less of something that will cause different behaviour.

As discussed offline, we'll enforce usage of the option and set callback as a field.

tritone · 2024-05-15T23:54:44Z

storage/transfermanager/downloader.go

-// Waits for all outstanding downloads to complete. The Downloader must not be
-// used to download more objects or directories after this has been called.
+// WaitAndClose waits for all outstanding downloads to complete. The Downloader
+// must not be used for any more downloads after this has been called.


Clearer to say that no new downloads can be added, perhaps? Also are we enforcing this somehow?

I think we should add unit tests for this behavior (and for stuff like the callback thing I mentioned).

We aren't enforcing it, but it should panic when trying to send on the closed channels. I can change the wording (and maybe also mention that it will panic) and add a test for it.

I changed this to error instead of panicking.

tritone · 2024-05-16T22:15:13Z

storage/transfermanager/downloader.go

-func (d *Downloader) Results() *DownloadOutputIterator {
-	return &DownloadOutputIterator{
-		output: d.output,
+// Results returns all the results of the downloads completed since the last


I think we could probably enforce that this can only be called after the full job is complete (given that we are keeping WaitAndClose as a single func). Or we could even just return the slice from WaitAndClose. What do you think?

I was thinking of just returning it in WaitAndClose, but hesitated given the async case - seems cleaner in that case to not return the results in WaitAndClose (since presumably it'd be empty).

We could enforce it only be called until the full job is complete by returning an error and/or an empty array if called before WaitAndClose... but then it may just be cleaner to return it in WaitAndClose. The way it is now should not cause any error if users do call it several times or before WaitAndClose, but could be confusing for some users. We could also always return the whole slice, but that has the same problems.

I think that, if we weigh both async and sync equally (and we don't want users grabbing results part way through) returning results in WaitAndClose is better, especially if we can assume people using async would be more aware of what they are doing and reading the docs, that would mention that results are empty for their case.

As discussed, I'll change this to return results directly in WaitAndClose

tritone · 2024-05-16T22:17:45Z

storage/transfermanager/downloader.go

+
+		// Keep track of any error that occurred.
+		if out.Err != nil {
+			d.error(out.Err)


Can we wrap this error to include the bucket/object name? I don't see that added in downloadShard either.

Sure! Technically they would have the information since it's in the output, but I see no harm including it here as well.

Sounds good; yeah if it's in the top-level output then you can see which individual result to inspect as well.

storage/transfermanager/integration_test.go

storage/transfermanager/option.go

tritone · 2024-05-16T22:32:12Z

storage/transfermanager/integration_test.go

+// errorIs is equivalent to errors.Is, except that it additionally will return
+// true if err and targetErr are googleapi.Errors with identical error codes,
+// or if both errors have the same gRPC status code.
+func errorIs(err error, targetErr error) bool {


Clever idea, we should probably do something like this for other tests which force errors.

You mean in the storage package? Yeah, I think we mostly do this type of check in the test functions themselves or don't check the exact returned err.

If we make a shared testutil we can add this to it!

tritone

Few docs notes, otherwise I think we should be able to submit this soon

storage/transfermanager/downloader.go

storage/transfermanager/example_test.go

tritone · 2024-05-24T04:23:38Z

storage/transfermanager/downloader.go

-	d.addInput(input)
-}
+// download but is non-blocking; call Downloader.Results or use the callback to
+// process the result.


We should explicitly note that this is intended to be thread-safe (can share the Downloader across goroutines). And add the note about timeouts/timing that you have in the example.

Done. Note I also made WaitAndClose callable more than once (just returns the same instead of panicking).

danielduhh · 2024-06-03T23:37:11Z

Is this ready to merge @BrennaEpp @tritone ?

tritone · 2024-06-04T16:54:51Z

storage/transfermanager/integration_test.go

+	}
+	ctx := context.Background()
+
+	tb.bucket = prefix + uidSpace.New()


I think we should talk about adding some more objects (at least more than the thread count) and of a wider variety of sizes. Can be in a follow up PR though.

tritone and others added 3 commits April 19, 2024 06:46

[WIP] feat(storage/transfermanager): prototype

bee724f

sketch of an interface for downloads

result iterator

189f87b

mvp, just missing some tests

a23db2e

product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Apr 25, 2024

tritone reviewed Apr 25, 2024

View reviewed changes

BrennaEpp added 4 commits May 10, 2024 14:00

wip - callbacks and sliced results

bbdd5a6

finish tests, some cleanup

9fcaca4

Merge branch 'main' into tm

6353ea3

revert go.work.sum

ca52f14

BrennaEpp marked this pull request as ready for review May 13, 2024 23:59

BrennaEpp requested review from a team as code owners May 14, 2024 00:01

BrennaEpp added 4 commits May 14, 2024 11:31

vet fixes

1d00ad4

Merge branch 'main' into tm

79d34f4

add grpc tests and errors.join for waitandclose

f5a6b33

remove bucket creation for short tests

8956b7b

BrennaEpp changed the title ~~draft(storage/transfermanager): prototype~~ feat(storage/transfermanager): prototype May 15, 2024

parse flags in test main to allow setup to skip in short mode

ed19c7f

tritone reviewed May 16, 2024

View reviewed changes

suggestions

5afae53

BrennaEpp requested a review from tritone May 22, 2024 18:26

tritone reviewed May 24, 2024

View reviewed changes

docs + allow waitandclose to be called more than once

5e42c62

BrennaEpp requested a review from tritone May 24, 2024 05:45

tritone approved these changes Jun 4, 2024

View reviewed changes

BrennaEpp enabled auto-merge (squash) June 4, 2024 23:54

Merge branch 'main' into tm

623fdd3

BrennaEpp merged commit cde5cbb into googleapis:main Jun 5, 2024
8 checks passed

release-please bot mentioned this pull request Jun 5, 2024

chore(main): release storage 1.42.0 #10217

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage/transfermanager): prototype #10045

feat(storage/transfermanager): prototype #10045

BrennaEpp commented Apr 25, 2024

tritone left a comment

tritone Apr 25, 2024

BrennaEpp Apr 27, 2024

tritone Apr 25, 2024

BrennaEpp Apr 25, 2024

tritone Apr 26, 2024

tritone Apr 25, 2024

BrennaEpp Apr 25, 2024

tritone Apr 26, 2024

tritone left a comment

tritone May 15, 2024

BrennaEpp May 23, 2024

tritone May 15, 2024

BrennaEpp May 16, 2024

BrennaEpp May 21, 2024

tritone May 15, 2024

BrennaEpp May 16, 2024

BrennaEpp May 22, 2024

tritone May 16, 2024

BrennaEpp May 16, 2024

BrennaEpp May 21, 2024

tritone May 16, 2024

BrennaEpp May 16, 2024

tritone May 17, 2024

tritone May 16, 2024

BrennaEpp May 16, 2024

tritone left a comment

tritone May 24, 2024

BrennaEpp May 24, 2024

danielduhh commented Jun 3, 2024

tritone Jun 4, 2024


		# Example usage

		// Pass in any client opts or set retry policy here.

feat(storage/transfermanager): prototype #10045

feat(storage/transfermanager): prototype #10045

Conversation

BrennaEpp commented Apr 25, 2024

tritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielduhh commented Jun 3, 2024

Choose a reason for hiding this comment