code review 180970043: google-api-go-client: Step 1 of supporting resuma...

google-api-go-client: Step 1 of supporting resumable uploads.
This creates the infrastructure needed to support resumable uploads.
The next step is to modify the generator and the APIs to enable users
to choose resumable uploads.

[gmlewis1, 2014-11-20 05:43:03]

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

[bradfitz, 2014-11-20 22:08:08]

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

[gmlewis1, 2014-11-20 22:54:53]

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.

[gmlewis1, 2014-11-20 23:31:46]

> 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.

[bradfitz, 2014-11-20 23:35:18]

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/
>

[gmlewis1, 2014-11-20 23:45:24]

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.

[bradfitz, 2014-11-21 00:25:31]

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/
>

[gmlewis1, 2014-11-21 06:03:14]

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.

[bradfitz, 2014-11-21 19:30:10]

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?

[bradfitz, 2014-11-21 19:44:17]

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/
>

[gmlewis1, 2014-11-21 20:02:46]

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).

[bradfitz, 2014-11-21 21:04:34]

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/
>

[gmlewis1, 2014-11-22 01:45:59]

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]).

[gmlewis1, 2014-12-02 19:43:24]

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).

[gmlewis1, 2014-12-02 23:39:28]

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".

[bradfitz, 2014-12-04 18:58:35]

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.

[gmlewis1, 2014-12-09 23:06:29]

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.

[bradfitz, 2014-12-10 06:05:14]

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/
>

[jbd, 2014-12-10 20:05:39]

+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/

[bradfitz, 2014-12-10 20:08:05]

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/
>

[bradfitz, 2014-12-11 01:13:16]

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/
>>
>
>

[gobot, 2014-12-19 05:17:07]

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.