|
|
Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/google-api-go-client
https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:281: // ResumableUpload is an interface used to provide resumable uploads. it's not an interface, though. Why would a user care or want to use this? Shouldn't they just say "upload" and have it work, even if it has to do this noise behind the scenes? https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:288: // MediaType defines the media type, e.g. "image/jpeg". Is this optional if I don't provide it? https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:290: // ChunkSize determines the size of the chunks to upload. 0 means don't chunk. Why is this configurable? And what does don't chunk mean? Does that mean upload it all at once? How is that resumable then? Can't you just pick a value or figure it out automatically? https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:293: ContentLength int64 can't this be inferred automatically from the Seeker? That's what http.ServeContent does at least. https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:294: // MaxRetries defines the maximum number of retries. If 0, the default used is 10. This also seems like a knob people won't care about. Just retry forever. They can use a timer if they want to give up after a certain period of time. Better might be an optional func hook to provide progress updates, so the caller knows if anything is happening (give byte updates every time a chunk makes it) https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:303: req.Header.Set("User-Agent", "google-api-go-client/0.5") use a constant, rather than hard-code this N times https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:311: if m := rangeRE.FindStringSubmatch(res.Header.Get("Range")); len(m) == 2 { m != nil by convention. you can trust len(m) == 2 if it matches at all https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:314: return 0, nil, fmt.Errorf("unable to parse range %v: %v", res.Header.Get("Range"), err) errors.New("range size overflows int64") seems sufficient or range size %v ... m[1]. But don't need to include all of Get("Range"), since you're not parsing the "0-" part https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:352: req.Header.Set("User-Agent", "google-api-go-client/0.5") again https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:383: return res, fmt.Errorf("unaligned_chunk error. Try using a power-of-two chunk size >= %v.", minGranularity) user shouldn't need to care about this
Thank you, Brad! PTAL. https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:281: // ResumableUpload is an interface used to provide resumable uploads. On 2014年11月20日 22:08:08, bradfitz wrote: > it's not an interface, though. > > Why would a user care or want to use this? Shouldn't they just say "upload" and > have it work, even if it has to do this noise behind the scenes? Right... a user should never care or want to use this. It is exported only for the generated APIs to use. I've changed the comment. https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:288: // MediaType defines the media type, e.g. "image/jpeg". On 2014年11月20日 22:08:07, bradfitz wrote: > Is this optional if I don't provide it? From the user's perspective, the media type continues to be optional, and the generated API will attempt to figure it out. However, the generated API must supply this information, as it has already figured out what type it is. https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:290: // ChunkSize determines the size of the chunks to upload. 0 means don't chunk. On 2014年11月20日 22:08:08, bradfitz wrote: > Why is this configurable? And what does don't chunk mean? Does that mean upload > it all at once? How is that resumable then? Can't you just pick a value or > figure it out automatically? I was planning on making an optional parameter "ChunkSize()" call so that users could customize it... thinking that maybe that would be the most flexible... but if you think I should pick a value, then I will probably go with 1<<18 since that is the minimum chunk size supported by Scotty. Yes, a chunkSize of 0 meant "upload it all at once"... not generally terribly useful. I'll hard-code it. https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:293: ContentLength int64 On 2014年11月20日 22:08:08, bradfitz wrote: > can't this be inferred automatically from the Seeker? That's what > http.ServeContent does at least. Yes, but the generated API has already performed the seeks and knows this length, so it does not need to be generated again. https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:294: // MaxRetries defines the maximum number of retries. If 0, the default used is 10. On 2014年11月20日 22:08:08, bradfitz wrote: > This also seems like a knob people won't care about. Just retry forever. They > can use a timer if they want to give up after a certain period of time. > > Better might be an optional func hook to provide progress updates, so the caller > knows if anything is happening (give byte updates every time a chunk makes it) Done. https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:303: req.Header.Set("User-Agent", "google-api-go-client/0.5") On 2014年11月20日 22:08:07, bradfitz wrote: > use a constant, rather than hard-code this N times Done. https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:311: if m := rangeRE.FindStringSubmatch(res.Header.Get("Range")); len(m) == 2 { On 2014年11月20日 22:08:07, bradfitz wrote: > m != nil by convention. you can trust len(m) == 2 if it matches at all I'm sorry... I don't understand this comment. I don't see where I'm checking "m != nil". https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:314: return 0, nil, fmt.Errorf("unable to parse range %v: %v", res.Header.Get("Range"), err) On 2014年11月20日 22:08:08, bradfitz wrote: > errors.New("range size overflows int64") seems sufficient > > or range size %v ... m[1]. But don't need to include all of Get("Range"), since > you're not parsing the "0-" part Done. https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:352: req.Header.Set("User-Agent", "google-api-go-client/0.5") On 2014年11月20日 22:08:08, bradfitz wrote: > again Done. https://codereview.appspot.com/180970043/diff/60001/googleapi/googleapi.go#ne... googleapi/googleapi.go:383: return res, fmt.Errorf("unaligned_chunk error. Try using a power-of-two chunk size >= %v.", minGranularity) On 2014年11月20日 22:08:07, bradfitz wrote: > user shouldn't need to care about this Done.
> On 2014年11月20日 22:08:08, bradfitz wrote: > > This also seems like a knob people won't care about. Just retry forever. They > > can use a timer if they want to give up after a certain period of time. > > > > Better might be an optional func hook to provide progress updates, so the > caller > > knows if anything is happening (give byte updates every time a chunk makes it) > > Done. Whups! I just realized that providing a "chan int64" is *not* a good solution! You said a "func hook", which makes a lot more sense. I'll work on that now.
Can you point me at some documentation for all this? What is it for? What will use it? What do other languages do? Example of how you'd use it from Go code, even if it's auto- generated? It's hard for me to think of a good API when I don't know what it is. On Nov 20, 2014 3:31 PM, <gmlewis@google.com> wrote: > On 2014年11月20日 22:08:08, bradfitz wrote: >> > This also seems like a knob people won't care about. Just retry >> > forever. They > >> > can use a timer if they want to give up after a certain period of >> > time. > >> > >> > Better might be an optional func hook to provide progress updates, >> > so the > >> caller >> > knows if anything is happening (give byte updates every time a chunk >> > makes it) > > Done. >> > > Whups! I just realized that providing a "chan int64" is *not* a good > solution! > You said a "func hook", which makes a lot more sense. I'll work on that > now. > > https://codereview.appspot.com/180970043/ >
There is no documentation for this yet, but I'm implementing this so that the Google Cloud Storage client library can support resumable uploads: https://github.com/GoogleCloudPlatform/gcloud-golang/issues/78 So, for example, instead of the GCS client library calling: resp, err := rawService(ctx).Objects.Insert(info.Bucket, info.toRawObject()).Media(w.rc).Do() it will call instead: resp, err := rawService(ctx).Objects.Insert(info.Bucket, info.toRawObject()).Name(info.Name).ResumableMedia(rs).Do() and uploads larger than 10MB will be supported. I've been manually testing this code with hand-modified storage-gen.go source, but thought that it might be easier to review the googleapi code first that is doing all the work before attempting to modify the generator that will provide the new functionality in all the APIs for media uploads. Please let me know if you would like me to show any of the hand-modified storage-gen.go source, but I'm thinking it is not terribly useful at this point.
Here's the API I'd like to see: resp, err := rawService(ctx).Objects.Insert(bucket, object).Media(myReader).Do() That's it. or maybe: resp, err := rawService(ctx).Objects.Insert(bucket, object).Media(myReaderSeeker).Do() or resp, err := rawService(ctx).Objects.Insert(bucket, object).Media(size, myReader).Do() And behind the scenes it buffers chunks as needed (before the server acks them), and internally does whatever Google-specific protocol is necessary to upload reliably with retries and chunks and Content-Range, etc. But no need to pollute the APIs with that. These "resumable uploads" aren't about a user working on an upload over several days after several reboots of their laptop or phone, right? If so, I don't see APIs about persisting and restoring the state (hopefully opaque) for that. So I assume this is for long-lived processes where the API from Go's side is blocking for the whole 1GB or whatever upload. And if that's the case, the code can be simple: just give it an io.Reader. How does the API descriptor JSON docs differentiate a media upload from a resumable media upload? On Thu, Nov 20, 2014 at 3:45 PM, <gmlewis@google.com> wrote: > There is no documentation for this yet, but I'm implementing this so > that the Google Cloud Storage client library can support resumable > uploads: > https://github.com/GoogleCloudPlatform/gcloud-golang/issues/78 > > So, for example, instead of the GCS client library calling: > > resp, err := rawService(ctx).Objects.Insert(info.Bucket, > info.toRawObject()).Media(w.rc).Do() > > it will call instead: > > resp, err := rawService(ctx).Objects.Insert(info.Bucket, > info.toRawObject()).Name(info.Name).ResumableMedia(rs).Do() > > and uploads larger than 10MB will be supported. > > I've been manually testing this code with hand-modified storage-gen.go > source, but thought that it might be easier to review the googleapi code > first that is doing all the work before attempting to modify the > generator that will provide the new functionality in all the APIs for > media uploads. > > Please let me know if you would like me to show any of the hand-modified > storage-gen.go source, but I'm thinking it is not terribly useful at > this point. > > > > https://codereview.appspot.com/180970043/ >
On 2014年11月21日 00:25:31, bradfitz wrote: > Here's the API I'd like to see: > > resp, err := rawService(ctx).Objects.Insert(bucket, > object).Media(myReader).Do() > > That's it. > > or maybe: > resp, err := rawService(ctx).Objects.Insert(bucket, > object).Media(myReaderSeeker).Do() Yes, that is exactly what I'm doing. The code you see here will be called *only* from the "Do()" function. (The only difference that you see is that I added a new "ResumableMedia()" call in the generated API, but we could make this the default... either way, it does not affect this code review.) This API here will only be used by the Do() function and will not be exposed to the developers. The Do() function is already doing a whole bunch of work that still needs to be done. > or > resp, err := rawService(ctx).Objects.Insert(bucket, object).Media(size, > myReader).Do() > > And behind the scenes it buffers chunks as needed (before the server acks > them), and internally does whatever Google-specific protocol is necessary > to upload reliably with retries and chunks and Content-Range, etc. But no > need to pollute the APIs with that. Yes, right. That's what I'm doing here. I'm not polluting the APIs... these functions will only be called by the Do() function. Step 2 will be to modify the generator which will modify the generated APIs and the Do() functions that will call this code. > These "resumable uploads" aren't about a user working on an upload over > several days after several reboots of their laptop or phone, right? If so, > I don't see APIs about persisting and restoring the state (hopefully > opaque) for that. So I assume this is for long-lived processes where the > API from Go's side is blocking for the whole 1GB or whatever upload. And if > that's the case, the code can be simple: just give it an io.Reader. Yes, that's what I'm doing... the latter... this is for long-lived processes blocking and sending potentially large uploads. The code in Do() remains simple and is given an io.ReadSeeker. That's it. > How does the API descriptor JSON docs differentiate a media upload from a > resumable media upload? We simply add "&uploadType=resumable"... this will happen in Step 2 - in the generated Do() function. I believe we are totally on the same page... if this is still unclear, please let me know and we can discuss it in person... It could be that maybe I'm just not explaining it very well.
https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:285: Client *http.Client if we don't expect to deal with redirect policy, this should just be be an http.RoundTripper. And named Transport. And needs docs, including what the zero value means (http.DefaultTransport probably). https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:286: // URI is the resumable resource provided by the server. I'm not sure what a "resumable resource" is. This is the target URL we PUT or POST to, right? I'd reword a bit to make it clear this is a destination and not a source. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:290: // MediaType defines the media type, e.g. "image/jpeg". what is the behavior if empty? https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:293: ContentLength int64 If Media is a Seeker, this can be dropped, right? https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:296: mu sync.Mutex mutex go before what they guard, with a blank line before, and often even a comment: type S struct { // other stuff mu sync.Mutex // guards progress progress int64 } https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:301: resumeIncomplete = 308 maybe preface it with "status" so it's similar to the Status constants in http://golang.org/pkg/net/http/#pkg-constants ? statusResumeIncomplete https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:310: chunkSize int64 = 1 << 18 no need for this to have a type I don't think? https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:314: func (rx *ResumableUpload) Progress() int64 { this is fine, but it does require users to poll, rather than get notified on progress. we could also have a callback func right? https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:361: body = bytes.NewBuffer(buf) bytes.NewReader https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:388: func (rx *ResumableUpload) Upload() (*http.Response, error) { should this take a context so users can cancel it? Or I guess they won't for now, since this is really internal? I wonder if we should actually put this into an "internal" package, now that that mechanism exists. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:398: expBackoff *= 2 does this ever reset back to 1 second after a success again?
http://godoc.org/code.google.com/p/google-api-go-client/storage/v1beta2#Objec... currently takes a Reader, not a ReadSeeker... we should probably keep that, so the ResumableUpload should only take a Reader too. But if we need to know the ContentLength (do we?), then we'd need an API change anyway. On Fri, Nov 21, 2014 at 11:30 AM, <bradfitz@golang.org> wrote: > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go > File googleapi/googleapi.go (right): > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode285 > googleapi/googleapi.go:285: Client *http.Client > if we don't expect to deal with redirect policy, this should just be be > an http.RoundTripper. And named Transport. And needs docs, including > what the zero value means (http.DefaultTransport probably). > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode286 > googleapi/googleapi.go:286: // URI is the resumable resource provided by > the server. > I'm not sure what a "resumable resource" is. This is the target URL we > PUT or POST to, right? I'd reword a bit to make it clear this is a > destination and not a source. > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode290 > googleapi/googleapi.go:290: // MediaType defines the media type, e.g. > "image/jpeg". > what is the behavior if empty? > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode293 > googleapi/googleapi.go:293: ContentLength int64 > If Media is a Seeker, this can be dropped, right? > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode296 > googleapi/googleapi.go:296: mu sync.Mutex > mutex go before what they guard, with a blank line before, and often > even a comment: > > type S struct { > // other stuff > > mu sync.Mutex // guards progress > progress int64 > } > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode301 > googleapi/googleapi.go:301: resumeIncomplete = 308 > maybe preface it with "status" so it's similar to the Status constants > in http://golang.org/pkg/net/http/#pkg-constants ? > statusResumeIncomplete > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode310 > googleapi/googleapi.go:310: chunkSize int64 = 1 << 18 > no need for this to have a type I don't think? > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode314 > googleapi/googleapi.go:314: func (rx *ResumableUpload) Progress() int64 > { > this is fine, but it does require users to poll, rather than get > notified on progress. we could also have a callback func right? > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode361 > googleapi/googleapi.go:361: body = bytes.NewBuffer(buf) > bytes.NewReader > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode388 > googleapi/googleapi.go:388: func (rx *ResumableUpload) Upload() > (*http.Response, error) { > should this take a context so users can cancel it? Or I guess they won't > for now, since this is really internal? > > I wonder if we should actually put this into an "internal" package, now > that that mechanism exists. > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode398 > googleapi/googleapi.go:398: expBackoff *= 2 > does this ever reset back to 1 second after a success again? > > https://codereview.appspot.com/180970043/ >
I'm replying to a subset of your comments... and will work on moving this to an "internal" package with a filename of "googleapi/internal/upload.go" unless you think it should be "googleapi/internal/internal.go" or some other name. I will also work on addressing your remaining comments. > http://godoc.org/code.google.com/p/google-api-go-client/storage/v1beta2#Objec... currently takes a Reader, not a ReadSeeker... we should probably keep that, so the ResumableUpload should only take a Reader too. But if we need to know the ContentLength (do we?), then we'd need an API change anyway. Yes, Media() still needs to take an io.Reader... But to support ResumableMedia() (or named "ResumableUpload" if you prefer), we need an io.ReadSeeker. It is not possible to support resumable uploads without the ability to seek to the last chunk that had an error unless we internally buffer the contents of the file, which I don't think we want to do. And yes, the ContentLength needs to be known for the transfer to work, as it is specified each time during the chunking. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:293: ContentLength int64 On 2014年11月21日 19:30:10, bradfitz wrote: > If Media is a Seeker, this can be dropped, right? This is an optimization and prevents the need for recomputing it every time that it is needed (which is often). https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:310: chunkSize int64 = 1 << 18 On 2014年11月21日 19:30:10, bradfitz wrote: > no need for this to have a type I don't think? If I don't declare it here as "int64", it becomes an "int" (since it is now a "var" for unit testing purposes) which would force me to cast it to "int64" down below on lines 354 and 355. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:398: expBackoff *= 2 On 2014年11月21日 19:30:10, bradfitz wrote: > does this ever reset back to 1 second after a success again? No, but I'm thinking that this is OK because "tx.transferChunks" will continuously upload a stream of chunks if no errors occur. It is only after the next error that expBackoff is increased... but then if the next attempted chunk is successful, it keeps streaming upload chunks at full speed (until the next error at which point it will pause before starting again).
I think buffering is fine. The buffering would be bounded at the size of a chunk: 256KB or even 1MB is fine. And if we're buffering, we can read the chunk to completion and know whether it's e.g. 256KB (a whole chunk, if that's the size we go with) or 11KB (e.g. the last chunk little bit of a file, if the file was 267KB). Does the upload protocol require us to declare the whole file length up front? Or only the size of each chunk? On Fri, Nov 21, 2014 at 12:02 PM, <gmlewis@google.com> wrote: > I'm replying to a subset of your comments... and will work on moving > this to an "internal" package with a filename of > "googleapi/internal/upload.go" unless you think it should be > "googleapi/internal/internal.go" or some other name. > I will also work on addressing your remaining comments. > > > http://godoc.org/code.google.com/p/google-api-go-client/storage/v1beta2# > ObjectsInsertCall.Media > currently takes a Reader, not a ReadSeeker... we should probably keep > that, so the ResumableUpload should only take a Reader too. But if we > need to know the ContentLength (do we?), then we'd need an API change > anyway. > > Yes, Media() still needs to take an io.Reader... > But to support ResumableMedia() (or named "ResumableUpload" if you > prefer), we need an io.ReadSeeker. > It is not possible to support resumable uploads without the ability to > seek to the last chunk that had an error unless we internally buffer the > contents of the file, which I don't think we want to do. > And yes, the ContentLength needs to be known for the transfer to work, > as it is specified each time during the chunking. > > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go > File googleapi/googleapi.go (right): > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode293 > googleapi/googleapi.go:293: ContentLength int64 > On 2014年11月21日 19:30:10, bradfitz wrote: > >> If Media is a Seeker, this can be dropped, right? >> > > This is an optimization and prevents the need for recomputing it every > time that it is needed (which is often). > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode310 > googleapi/googleapi.go:310: chunkSize int64 = 1 << 18 > On 2014年11月21日 19:30:10, bradfitz wrote: > >> no need for this to have a type I don't think? >> > > If I don't declare it here as "int64", it becomes an "int" (since it is > now a "var" for unit testing purposes) which would force me to cast it > to "int64" down below on lines 354 and 355. > > https://codereview.appspot.com/180970043/diff/180001/ > googleapi/googleapi.go#newcode398 > googleapi/googleapi.go:398: expBackoff *= 2 > On 2014年11月21日 19:30:10, bradfitz wrote: > >> does this ever reset back to 1 second after a success again? >> > > No, but I'm thinking that this is OK because "tx.transferChunks" will > continuously upload a stream of chunks if no errors occur. > It is only after the next error that expBackoff is increased... but then > if the next attempted chunk is successful, it keeps streaming upload > chunks at full speed (until the next error at which point it will pause > before starting again). > > https://codereview.appspot.com/180970043/ >
On 2014年11月21日 21:04:34, bradfitz wrote: > I think buffering is fine. The buffering would be bounded at the size of a > chunk: 256KB or even 1MB is fine. > > And if we're buffering, we can read the chunk to completion and know > whether it's e.g. 256KB (a whole chunk, if that's the size we go with) or > 11KB (e.g. the last chunk little bit of a file, if the file was 267KB). > > Does the upload protocol require us to declare the whole file length up > front? Or only the size of each chunk? Yes, we must specify the whole file length up-front. So we either need to: 1) get an io.Reader plus the full content length, or 2) get an io.ReadSeeker. I discussed this with Burcu, and it seemed like the best thing to do would be to accept an io.ReadSeeker since the API must change anyway. Additionally, I'm not sure that we can guarantee that failures will *only* be for the last chunk sent, although I would hope so. If we are willing to risk that an upload failure will always be constrained to the last-sent chunk, then we could do option #1, but it seems like option #2 gives us the ability to seek back to any location that the server forces us to jump to. Your call... my vote is for #2. Either way, I can add buffering of the last chunk (with the buffer size being the minimum of [chunkSize or totalContentLength]).
Reading through the docs for "internal" packages here: http://tip.golang.org/doc/go1.4#internalpackages it looks like placing this code in an "internal" subdirectory would actually break it because it needs to be imported by the auto-generated APIs (which do not live under the "googleapi" path).
I believe I have addressed all your comments and implemented the buffering (but could not move to an internal package... see below). PTAL. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:285: Client *http.Client On 2014年11月21日 19:30:10, bradfitz wrote: > if we don't expect to deal with redirect policy, this should just be be an > http.RoundTripper. And named Transport. And needs docs, including what the zero > value means (http.DefaultTransport probably). I don't understand this comment. The auto-generated API already has an http.Client and all we need to do here is call its "Do" function. A developer will never use this directly... only auto-generated APIs will use this, and they have their own mechanisms for creating the http.Client. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:286: // URI is the resumable resource provided by the server. On 2014年11月21日 19:30:10, bradfitz wrote: > I'm not sure what a "resumable resource" is. This is the target URL we PUT or > POST to, right? I'd reword a bit to make it clear this is a destination and not > a source. Done. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:290: // MediaType defines the media type, e.g. "image/jpeg". On 2014年11月21日 19:30:10, bradfitz wrote: > what is the behavior if empty? This will never be empty, as it is provided by the auto-generated API. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:296: mu sync.Mutex On 2014年11月21日 19:30:10, bradfitz wrote: > mutex go before what they guard, with a blank line before, and often even a > comment: > > type S struct { > // other stuff > > mu sync.Mutex // guards progress > progress int64 > } Done. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:301: resumeIncomplete = 308 On 2014年11月21日 19:30:10, bradfitz wrote: > maybe preface it with "status" so it's similar to the Status constants in > http://golang.org/pkg/net/http/#pkg-constants ? statusResumeIncomplete Done. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:314: func (rx *ResumableUpload) Progress() int64 { On 2014年11月21日 19:30:10, bradfitz wrote: > this is fine, but it does require users to poll, rather than get notified on > progress. we could also have a callback func right? Done. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:361: body = bytes.NewBuffer(buf) On 2014年11月21日 19:30:10, bradfitz wrote: > bytes.NewReader Done. https://codereview.appspot.com/180970043/diff/180001/googleapi/googleapi.go#n... googleapi/googleapi.go:388: func (rx *ResumableUpload) Upload() (*http.Response, error) { On 2014年11月21日 19:30:10, bradfitz wrote: > should this take a context so users can cancel it? Or I guess they won't for > now, since this is really internal? > > I wonder if we should actually put this into an "internal" package, now that > that mechanism exists. I'll hold off on the cancel feature for now, since this is internal. As for moving to "internal", it appears that we can't because the auto-generated APIs are not in the same directory as "googleapi".
https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:288: // It is not used by developers directly. If this isn't usable by developers directly, what is ProgressUpdater for? How will users interact with this? Could you add one caller to this CL so I can see it end-to-end? https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:294: Media io.ReadSeeker I think this would all be much simpler if this were an io.ReaderAt instead. Then you can just make new io.NewSectionReader out of it and ditch the generator goroutine. It also leaves open the possibility to have multiple chunks being uploaded at once in the future, if the server supports that. https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:312: userAgent = "google-api-go-client/0.5" this feels out of place. Where is this constant already? It's somewhere, no? I'd put it at the top, and unify all the constants. https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:317: rangeRE = regexp.MustCompile(`^0\-(\d+)$`) why is it always zero? The comment should say what 1ドル is supposed to be and how this is used more. You can remove that it's a regular expression. "rangeRE matches the foo from the bar. 1ドル means blah." https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:319: chunkSize int64 = 1 << 18 this doesn't need to have a type, does it? Also, document why this value. e.g. minimum or maximum supported by the server? good balance between foo and bar? https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:426: func (rx *ResumableUpload) Upload() (*http.Response, error) { what if this took a contest.Context instead, so users can do their own cancellation, rather than hard-code 60 seconds. then loop forever until they cancel. that also gives you a place to let them attach something like ProgressUpdater in the future. Or we can make ProgressUpdater a supported Context Key (possibly in a future CL) but make the ResumableUpload private. I don't know how it will be used yet from the user-facing API package.
https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:288: // It is not used by developers directly. On 2014年12月04日 18:58:35, bradfitz wrote: > If this isn't usable by developers directly, what is ProgressUpdater for? How > will users interact with this? > > Could you add one caller to this CL so I can see it end-to-end? I've improved the comments and added an example in the unit tests. https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:294: Media io.ReadSeeker On 2014年12月04日 18:58:35, bradfitz wrote: > I think this would all be much simpler if this were an io.ReaderAt instead. > Then you can just make new io.NewSectionReader out of it and ditch the generator > goroutine. It also leaves open the possibility to have multiple chunks being > uploaded at once in the future, if the server supports that. Done. https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:312: userAgent = "google-api-go-client/0.5" On 2014年12月04日 18:58:34, bradfitz wrote: > this feels out of place. > > Where is this constant already? It's somewhere, no? > > I'd put it at the top, and unify all the constants. Yes... this is a really weird constant... I found it in the GCS code... but a Google search surfaces it in all sorts of locations: https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&es_th=1&ie=... even camlistore: https://camlistore.googlesource.com/camlistore/+/829086f553a262bab9fa5f1f153a... https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:317: rangeRE = regexp.MustCompile(`^0\-(\d+)$`) On 2014年12月04日 18:58:35, bradfitz wrote: > why is it always zero? The comment should say what 1ドル is supposed to be and how > this is used more. You can remove that it's a regular expression. "rangeRE > matches the foo from the bar. 1ドル means blah." It starts with zero because the server only responds that way and we continue a resumable upload by adding to it (starting from zero). Done. https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:319: chunkSize int64 = 1 << 18 On 2014年12月04日 18:58:35, bradfitz wrote: > this doesn't need to have a type, does it? Also, document why this value. e.g. > minimum or maximum supported by the server? good balance between foo and bar? If I don't declare it here as "int64", it becomes an "int" which would force me to cast it to "int64" down below in multiple locations. https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... googleapi/googleapi.go:426: func (rx *ResumableUpload) Upload() (*http.Response, error) { On 2014年12月04日 18:58:34, bradfitz wrote: > what if this took a contest.Context instead, so users can do their own > cancellation, rather than hard-code 60 seconds. then loop forever until they > cancel. > > that also gives you a place to let them attach something like ProgressUpdater in > the future. Or we can make ProgressUpdater a supported Context Key (possibly in > a future CL) but make the ResumableUpload private. I don't know how it will be > used yet from the user-facing API package. OK, I'm working on this now... but since this is a non-trivial change, I figured I would just address the other comments first and publish an update.
Today we (with much pain) moved this project from hg to git. It now lives at code.Googlesource.com/google-api-go-client Can you resend this review on Gerrit using the new Go & git-review contribution process? I can help you tomorrow (Sydney) morning if you hit problems. Note we haven't moved the issues anywhere yet. Thanks to David Symonds for fighting hg and git and gerrit with me. On Dec 10, 2014 10:06 AM, <gmlewis@google.com> wrote: > > https://codereview.appspot.com/180970043/diff/220001/ > googleapi/googleapi.go > File googleapi/googleapi.go (right): > > https://codereview.appspot.com/180970043/diff/220001/ > googleapi/googleapi.go#newcode288 > googleapi/googleapi.go:288: // It is not used by developers directly. > On 2014年12月04日 18:58:35, bradfitz wrote: > >> If this isn't usable by developers directly, what is ProgressUpdater >> > for? How > >> will users interact with this? >> > > Could you add one caller to this CL so I can see it end-to-end? >> > > I've improved the comments and added an example in the unit tests. > > https://codereview.appspot.com/180970043/diff/220001/ > googleapi/googleapi.go#newcode294 > googleapi/googleapi.go:294: Media io.ReadSeeker > On 2014年12月04日 18:58:35, bradfitz wrote: > >> I think this would all be much simpler if this were an io.ReaderAt >> > instead. > >> Then you can just make new io.NewSectionReader out of it and ditch the >> > generator > >> goroutine. It also leaves open the possibility to have multiple >> > chunks being > >> uploaded at once in the future, if the server supports that. >> > > Done. > > https://codereview.appspot.com/180970043/diff/220001/ > googleapi/googleapi.go#newcode312 > googleapi/googleapi.go:312: userAgent = "google-api-go-client/0.5" > On 2014年12月04日 18:58:34, bradfitz wrote: > >> this feels out of place. >> > > Where is this constant already? It's somewhere, no? >> > > I'd put it at the top, and unify all the constants. >> > > Yes... this is a really weird constant... I found it in the GCS code... > but a Google search surfaces it in all sorts of locations: > https://www.google.com/webhp?sourceid=chrome-instant&ion=1& > espv=2&es_th=1&ie=UTF-8#sourceid=chrome-psyapi2&es_th= > 1&ie=UTF-8&q=google-api-go-client%2F0.5 > even camlistore: > https://camlistore.googlesource.com/camlistore/+/ > 829086f553a262bab9fa5f1f153afddc8bd75515/third_party/code. > google.com/p/google-api-go-client/drive/v2/drive-gen.go#1312 > > https://codereview.appspot.com/180970043/diff/220001/ > googleapi/googleapi.go#newcode317 > googleapi/googleapi.go:317: rangeRE = regexp.MustCompile(`^0\-(\d+)$`) > On 2014年12月04日 18:58:35, bradfitz wrote: > >> why is it always zero? The comment should say what 1ドル is supposed to >> > be and how > >> this is used more. You can remove that it's a regular expression. >> > "rangeRE > >> matches the foo from the bar. 1ドル means blah." >> > > It starts with zero because the server only responds that way and we > continue a resumable upload by adding to it (starting from zero). > Done. > > https://codereview.appspot.com/180970043/diff/220001/ > googleapi/googleapi.go#newcode319 > googleapi/googleapi.go:319: chunkSize int64 = 1 << 18 > On 2014年12月04日 18:58:35, bradfitz wrote: > >> this doesn't need to have a type, does it? Also, document why this >> > value. e.g. > >> minimum or maximum supported by the server? good balance between foo >> > and bar? > > If I don't declare it here as "int64", it becomes an "int" which would > force me to cast it to "int64" down > below in multiple locations. > > https://codereview.appspot.com/180970043/diff/220001/ > googleapi/googleapi.go#newcode426 > googleapi/googleapi.go:426: func (rx *ResumableUpload) Upload() > (*http.Response, error) { > On 2014年12月04日 18:58:34, bradfitz wrote: > >> what if this took a contest.Context instead, so users can do their own >> cancellation, rather than hard-code 60 seconds. then loop forever >> > until they > >> cancel. >> > > that also gives you a place to let them attach something like >> > ProgressUpdater in > >> the future. Or we can make ProgressUpdater a supported Context Key >> > (possibly in > >> a future CL) but make the ResumableUpload private. I don't know how >> > it will be > >> used yet from the user-facing API package. >> > > OK, I'm working on this now... but since this is a non-trivial change, I > figured I would just address the other comments first and publish an > update. > > https://codereview.appspot.com/180970043/ >
+proppy As far as I remember, you were considering a vanity import path for this package. Could you set it up so I can migrate? On Tue, Dec 9, 2014 at 10:05 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Today we (with much pain) moved this project from hg to git. It now lives at > code.Googlesource.com/google-api-go-client > > Can you resend this review on Gerrit using the new Go & git-review > contribution process? > > I can help you tomorrow (Sydney) morning if you hit problems. > > Note we haven't moved the issues anywhere yet. > > Thanks to David Symonds for fighting hg and git and gerrit with me. > > On Dec 10, 2014 10:06 AM, <gmlewis@google.com> wrote: >> >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go >> File googleapi/googleapi.go (right): >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> googleapi/googleapi.go:288: // It is not used by developers directly. >> On 2014年12月04日 18:58:35, bradfitz wrote: >>> >>> If this isn't usable by developers directly, what is ProgressUpdater >> >> for? How >>> >>> will users interact with this? >> >> >>> Could you add one caller to this CL so I can see it end-to-end? >> >> >> I've improved the comments and added an example in the unit tests. >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> googleapi/googleapi.go:294: Media io.ReadSeeker >> On 2014年12月04日 18:58:35, bradfitz wrote: >>> >>> I think this would all be much simpler if this were an io.ReaderAt >> >> instead. >>> >>> Then you can just make new io.NewSectionReader out of it and ditch the >> >> generator >>> >>> goroutine. It also leaves open the possibility to have multiple >> >> chunks being >>> >>> uploaded at once in the future, if the server supports that. >> >> >> Done. >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> googleapi/googleapi.go:312: userAgent = "google-api-go-client/0.5" >> On 2014年12月04日 18:58:34, bradfitz wrote: >>> >>> this feels out of place. >> >> >>> Where is this constant already? It's somewhere, no? >> >> >>> I'd put it at the top, and unify all the constants. >> >> >> Yes... this is a really weird constant... I found it in the GCS code... >> but a Google search surfaces it in all sorts of locations: >> >> https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&es_th=1&ie=... >> even camlistore: >> >> https://camlistore.googlesource.com/camlistore/+/829086f553a262bab9fa5f1f153a... >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> googleapi/googleapi.go:317: rangeRE = regexp.MustCompile(`^0\-(\d+)$`) >> On 2014年12月04日 18:58:35, bradfitz wrote: >>> >>> why is it always zero? The comment should say what 1ドル is supposed to >> >> be and how >>> >>> this is used more. You can remove that it's a regular expression. >> >> "rangeRE >>> >>> matches the foo from the bar. 1ドル means blah." >> >> >> It starts with zero because the server only responds that way and we >> continue a resumable upload by adding to it (starting from zero). >> Done. >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> googleapi/googleapi.go:319: chunkSize int64 = 1 << 18 >> On 2014年12月04日 18:58:35, bradfitz wrote: >>> >>> this doesn't need to have a type, does it? Also, document why this >> >> value. e.g. >>> >>> minimum or maximum supported by the server? good balance between foo >> >> and bar? >> >> If I don't declare it here as "int64", it becomes an "int" which would >> force me to cast it to "int64" down >> below in multiple locations. >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> googleapi/googleapi.go:426: func (rx *ResumableUpload) Upload() >> (*http.Response, error) { >> On 2014年12月04日 18:58:34, bradfitz wrote: >>> >>> what if this took a contest.Context instead, so users can do their own >>> cancellation, rather than hard-code 60 seconds. then loop forever >> >> until they >>> >>> cancel. >> >> >>> that also gives you a place to let them attach something like >> >> ProgressUpdater in >>> >>> the future. Or we can make ProgressUpdater a supported Context Key >> >> (possibly in >>> >>> a future CL) but make the ResumableUpload private. I don't know how >> >> it will be >>> >>> used yet from the user-facing API package. >> >> >> OK, I'm working on this now... but since this is a non-trivial change, I >> figured I would just address the other comments first and publish an >> update. >> >> https://codereview.appspot.com/180970043/
Yup. We started to do it yesterday but ran out of time. It's on our list for today. On Wed, Dec 10, 2014 at 12:05 PM, Burcu Dogan <jbd@google.com> wrote: > +proppy > > As far as I remember, you were considering a vanity import path for > this package. > > Could you set it up so I can migrate? > > On Tue, Dec 9, 2014 at 10:05 PM, Brad Fitzpatrick <bradfitz@golang.org> > wrote: > > Today we (with much pain) moved this project from hg to git. It now > lives at > > code.Googlesource.com/google-api-go-client > > > > Can you resend this review on Gerrit using the new Go & git-review > > contribution process? > > > > I can help you tomorrow (Sydney) morning if you hit problems. > > > > Note we haven't moved the issues anywhere yet. > > > > Thanks to David Symonds for fighting hg and git and gerrit with me. > > > > On Dec 10, 2014 10:06 AM, <gmlewis@google.com> wrote: > >> > >> > >> > >> > https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go > >> File googleapi/googleapi.go (right): > >> > >> > >> > https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... > >> googleapi/googleapi.go:288: // It is not used by developers directly. > >> On 2014年12月04日 18:58:35, bradfitz wrote: > >>> > >>> If this isn't usable by developers directly, what is ProgressUpdater > >> > >> for? How > >>> > >>> will users interact with this? > >> > >> > >>> Could you add one caller to this CL so I can see it end-to-end? > >> > >> > >> I've improved the comments and added an example in the unit tests. > >> > >> > >> > https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... > >> googleapi/googleapi.go:294: Media io.ReadSeeker > >> On 2014年12月04日 18:58:35, bradfitz wrote: > >>> > >>> I think this would all be much simpler if this were an io.ReaderAt > >> > >> instead. > >>> > >>> Then you can just make new io.NewSectionReader out of it and ditch the > >> > >> generator > >>> > >>> goroutine. It also leaves open the possibility to have multiple > >> > >> chunks being > >>> > >>> uploaded at once in the future, if the server supports that. > >> > >> > >> Done. > >> > >> > >> > https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... > >> googleapi/googleapi.go:312: userAgent = "google-api-go-client/0.5" > >> On 2014年12月04日 18:58:34, bradfitz wrote: > >>> > >>> this feels out of place. > >> > >> > >>> Where is this constant already? It's somewhere, no? > >> > >> > >>> I'd put it at the top, and unify all the constants. > >> > >> > >> Yes... this is a really weird constant... I found it in the GCS code... > >> but a Google search surfaces it in all sorts of locations: > >> > >> > https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&es_th=1&ie=... > >> even camlistore: > >> > >> > https://camlistore.googlesource.com/camlistore/+/829086f553a262bab9fa5f1f153a... > >> > >> > >> > https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... > >> googleapi/googleapi.go:317: rangeRE = regexp.MustCompile(`^0\-(\d+)$`) > >> On 2014年12月04日 18:58:35, bradfitz wrote: > >>> > >>> why is it always zero? The comment should say what 1ドル is supposed to > >> > >> be and how > >>> > >>> this is used more. You can remove that it's a regular expression. > >> > >> "rangeRE > >>> > >>> matches the foo from the bar. 1ドル means blah." > >> > >> > >> It starts with zero because the server only responds that way and we > >> continue a resumable upload by adding to it (starting from zero). > >> Done. > >> > >> > >> > https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... > >> googleapi/googleapi.go:319: chunkSize int64 = 1 << 18 > >> On 2014年12月04日 18:58:35, bradfitz wrote: > >>> > >>> this doesn't need to have a type, does it? Also, document why this > >> > >> value. e.g. > >>> > >>> minimum or maximum supported by the server? good balance between foo > >> > >> and bar? > >> > >> If I don't declare it here as "int64", it becomes an "int" which would > >> force me to cast it to "int64" down > >> below in multiple locations. > >> > >> > >> > https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... > >> googleapi/googleapi.go:426: func (rx *ResumableUpload) Upload() > >> (*http.Response, error) { > >> On 2014年12月04日 18:58:34, bradfitz wrote: > >>> > >>> what if this took a contest.Context instead, so users can do their own > >>> cancellation, rather than hard-code 60 seconds. then loop forever > >> > >> until they > >>> > >>> cancel. > >> > >> > >>> that also gives you a place to let them attach something like > >> > >> ProgressUpdater in > >>> > >>> the future. Or we can make ProgressUpdater a supported Context Key > >> > >> (possibly in > >>> > >>> a future CL) but make the ResumableUpload private. I don't know how > >> > >> it will be > >>> > >>> used yet from the user-facing API package. > >> > >> > >> OK, I'm working on this now... but since this is a non-trivial change, I > >> figured I would just address the other comments first and publish an > >> update. > >> > >> https://codereview.appspot.com/180970043/ >
It is now done. On Thu, Dec 11, 2014 at 7:08 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Yup. We started to do it yesterday but ran out of time. It's on our list > for today. > > > On Wed, Dec 10, 2014 at 12:05 PM, Burcu Dogan <jbd@google.com> wrote: > >> +proppy >> >> As far as I remember, you were considering a vanity import path for >> this package. >> >> Could you set it up so I can migrate? >> >> On Tue, Dec 9, 2014 at 10:05 PM, Brad Fitzpatrick <bradfitz@golang.org> >> wrote: >> > Today we (with much pain) moved this project from hg to git. It now >> lives at >> > code.Googlesource.com/google-api-go-client >> > >> > Can you resend this review on Gerrit using the new Go & git-review >> > contribution process? >> > >> > I can help you tomorrow (Sydney) morning if you hit problems. >> > >> > Note we haven't moved the issues anywhere yet. >> > >> > Thanks to David Symonds for fighting hg and git and gerrit with me. >> > >> > On Dec 10, 2014 10:06 AM, <gmlewis@google.com> wrote: >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go >> >> File googleapi/googleapi.go (right): >> >> >> >> >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> >> googleapi/googleapi.go:288: // It is not used by developers directly. >> >> On 2014年12月04日 18:58:35, bradfitz wrote: >> >>> >> >>> If this isn't usable by developers directly, what is ProgressUpdater >> >> >> >> for? How >> >>> >> >>> will users interact with this? >> >> >> >> >> >>> Could you add one caller to this CL so I can see it end-to-end? >> >> >> >> >> >> I've improved the comments and added an example in the unit tests. >> >> >> >> >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> >> googleapi/googleapi.go:294: Media io.ReadSeeker >> >> On 2014年12月04日 18:58:35, bradfitz wrote: >> >>> >> >>> I think this would all be much simpler if this were an io.ReaderAt >> >> >> >> instead. >> >>> >> >>> Then you can just make new io.NewSectionReader out of it and ditch the >> >> >> >> generator >> >>> >> >>> goroutine. It also leaves open the possibility to have multiple >> >> >> >> chunks being >> >>> >> >>> uploaded at once in the future, if the server supports that. >> >> >> >> >> >> Done. >> >> >> >> >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> >> googleapi/googleapi.go:312: userAgent = "google-api-go-client/0.5" >> >> On 2014年12月04日 18:58:34, bradfitz wrote: >> >>> >> >>> this feels out of place. >> >> >> >> >> >>> Where is this constant already? It's somewhere, no? >> >> >> >> >> >>> I'd put it at the top, and unify all the constants. >> >> >> >> >> >> Yes... this is a really weird constant... I found it in the GCS code... >> >> but a Google search surfaces it in all sorts of locations: >> >> >> >> >> https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&es_th=1&ie=... >> >> even camlistore: >> >> >> >> >> https://camlistore.googlesource.com/camlistore/+/829086f553a262bab9fa5f1f153a... >> >> >> >> >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> >> googleapi/googleapi.go:317: rangeRE = regexp.MustCompile(`^0\-(\d+)$`) >> >> On 2014年12月04日 18:58:35, bradfitz wrote: >> >>> >> >>> why is it always zero? The comment should say what 1ドル is supposed to >> >> >> >> be and how >> >>> >> >>> this is used more. You can remove that it's a regular expression. >> >> >> >> "rangeRE >> >>> >> >>> matches the foo from the bar. 1ドル means blah." >> >> >> >> >> >> It starts with zero because the server only responds that way and we >> >> continue a resumable upload by adding to it (starting from zero). >> >> Done. >> >> >> >> >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> >> googleapi/googleapi.go:319: chunkSize int64 = 1 << 18 >> >> On 2014年12月04日 18:58:35, bradfitz wrote: >> >>> >> >>> this doesn't need to have a type, does it? Also, document why this >> >> >> >> value. e.g. >> >>> >> >>> minimum or maximum supported by the server? good balance between foo >> >> >> >> and bar? >> >> >> >> If I don't declare it here as "int64", it becomes an "int" which would >> >> force me to cast it to "int64" down >> >> below in multiple locations. >> >> >> >> >> >> >> https://codereview.appspot.com/180970043/diff/220001/googleapi/googleapi.go#n... >> >> googleapi/googleapi.go:426: func (rx *ResumableUpload) Upload() >> >> (*http.Response, error) { >> >> On 2014年12月04日 18:58:34, bradfitz wrote: >> >>> >> >>> what if this took a contest.Context instead, so users can do their own >> >>> cancellation, rather than hard-code 60 seconds. then loop forever >> >> >> >> until they >> >>> >> >>> cancel. >> >> >> >> >> >>> that also gives you a place to let them attach something like >> >> >> >> ProgressUpdater in >> >>> >> >>> the future. Or we can make ProgressUpdater a supported Context Key >> >> >> >> (possibly in >> >>> >> >>> a future CL) but make the ResumableUpload private. I don't know how >> >> >> >> it will be >> >>> >> >>> used yet from the user-facing API package. >> >> >> >> >> >> OK, I'm working on this now... but since this is a non-trivial change, >> I >> >> figured I would just address the other comments first and publish an >> >> update. >> >> >> >> https://codereview.appspot.com/180970043/ >> > >
R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/180970043 is best) in the description in your new CL. Thanks very much.