Implement retry flow on DriveUploader.

Implement retry flow on DriveUploader.

This CL implements the retrying feature on DriveUploader when the server
returns 5xx server error response.
This is the preparation of switching to the stream data upload with increasing
the chunk size.

BUG=148632
TEST=Ran unit_tests.

[hidehiko, 2013-02-06 15:10:47]

This CL depends on https://codereview.chromium.org/12246002/,
which is also sent to the code review.

FYI: the CLs
https://codereview.chromium.org/12235002/,
https://codereview.chromium.org/12224026/,
https://codereview.chromium.org/12246002/ and this CL implement retry for 5xx
server error on DriveUploader.

I hope sending them at once illustrates what I'd like to change eventually
(following the Ryo's comment on cl 12235002).

Thank you for your review in advance,
- hidehiko

[hidehiko, 2013-02-06 15:11:25]

This CL depends on https://codereview.chromium.org/12246002/,
which is also sent to the code review.

FYI: the CLs
https://codereview.chromium.org/12235002/,
https://codereview.chromium.org/12224026/,
https://codereview.chromium.org/12246002/ and this CL implement retry for 5xx
server error on DriveUploader.

I hope sending them at once illustrates what I'd like to change eventually
(following the Ryo's comment on cl 12235002).

Thank you for your review in advance,
- hidehiko

[satorux1, 2013-02-07 05:48:32]

I'd defer to kinaba@ who knows more about the upload code. :)

Added zork@ for the backoff logic.

https://codereview.chromium.org/12209035/diff/8/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_uploader.cc (right):

https://codereview.chromium.org/12209035/diff/8/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_uploader.cc:51: // for more details.
I thought we had similar code elsewhere. zork@ what do you think?

https://codereview.chromium.org/12209035/diff/8/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_uploader.h (right):

https://codereview.chromium.org/12209035/diff/8/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_uploader.h:127: // Call DriveServer's
ResumeUpload with the current remaining chunk of data.
Calls ?

[Zachary Kuznia, 2013-02-07 06:26:36]

https://codereview.chromium.org/12209035/diff/8/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_uploader.cc (right):

https://codereview.chromium.org/12209035/diff/8/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_uploader.cc:51: // for more details.
On 2013/02/07 05:48:32, satorux1 wrote:
> I thought we had similar code elsewhere. zork@ what do you think?

This should call into the scheduler instead.  That will both handle retry logic
for throttled cases, and suspend the operation when the user turns off drive or
switches to LTE.

The CL that makes the uploader wrapped by the scheduler is in progress.  I'll
update and reupload it presently.

[hidehiko, 2013-02-07 06:53:26]

https://codereview.chromium.org/12209035/diff/8/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_uploader.cc (right):

https://codereview.chromium.org/12209035/diff/8/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_uploader.cc:51: // for more details.
On 2013/02/07 06:26:36, Zachary Kuznia wrote:
> On 2013/02/07 05:48:32, satorux1 wrote:
> > I thought we had similar code elsewhere. zork@ what do you think?
> 
> This should call into the scheduler instead.  That will both handle retry
logic
> for throttled cases, and suspend the operation when the user turns off drive
or
> switches to LTE.
> 
> The CL that makes the uploader wrapped by the scheduler is in progress.  I'll
> update and reupload it presently.

So, all retrying logic will be in DriveScheduler somehow soon, right?
Sounds great, but then I have two questions:

1) DriveUploader is a state machine based on Chrome's message queue, and it is
completely independent from the DriveScheduler at the moment. How will they
communicate with each other? In particular, I'm interested in how to manage the
uploading state and to re-construct it when http-connection is failed, because
I'm about sending another CL to change it to more efficient way.

2) DriveUploader is used not only from chrome/browser/chromeos/drive/... but
also chrome/browser/sync_file_system/... What's the plan for it?

[Zachary Kuznia, 2013-02-07 10:17:18]

> 1) DriveUploader is a state machine based on Chrome's message queue, and it is
> completely independent from the DriveScheduler at the moment. How will they
> communicate with each other? In particular, I'm interested in how to manage
the
> uploading state and to re-construct it when http-connection is failed, because
> I'm about sending another CL to change it to more efficient way.

I think we'll need to add a concept of resuming an operation to the scheduler to
handle this.
 
> 2) DriveUploader is used not only from chrome/browser/chromeos/drive/... but
> also chrome/browser/sync_file_system/... What's the plan for it?

This is a potential problem for all operations that call directly into the drive
service.  The right answer will probably be to move the scheduler to google_apis
eventually.

[hidehiko, 2013-02-08 08:45:27]

On 2013/02/07 10:17:18, Zachary Kuznia wrote:
> > 1) DriveUploader is a state machine based on Chrome's message queue, and it
is
> > completely independent from the DriveScheduler at the moment. How will they
> > communicate with each other? In particular, I'm interested in how to manage
> the
> > uploading state and to re-construct it when http-connection is failed,
because
> > I'm about sending another CL to change it to more efficient way.
> 
> I think we'll need to add a concept of resuming an operation to the scheduler
to
> handle this.
>  
> > 2) DriveUploader is used not only from chrome/browser/chromeos/drive/... but
> > also chrome/browser/sync_file_system/... What's the plan for it?
> 
> This is a potential problem for all operations that call directly into the
drive
> service.  The right answer will probably be to move the scheduler to
google_apis
> eventually.

For the record:

Zach, talked to Satoru and Kazuhiro offline more. Then the plan is:
Create a public method to resume the interrupted upload in DriveUploader (e.g.
DriveUploader::ResumeInterruptedUpload or something?), and DriveScheduler will
have the responsibility to call it if the server returns an error.

Detailed scenario:
If the server returns an error, DriveScheduler observes it (by passing through
the DriveUploader). On the current implementation, it will restart the uploading
"from beginning".
Instead, let DriveScheduler enqueue a job to call
DriveUpload::ResumeInterruptedUpload with appropriate parameters, and run the
queue again (with some throttling wait).

By this change,
- DriveScheduler will keep the responsibility to manage the jobs with throttle
handling
- DriveUploader will keep the responsibility to manage the uploading data.
- No dependency from DriveUploader to DriveScheduler.

Note that DriveUploader will send a sequence of http requests, until it fails
(but will be reduced to just two, when "whole file transfer at once" is
implemented).

What do you think?
If it's ok for you, too, let me work on that step by step.

[Zachary Kuznia, 2013-02-12 01:47:02]

This sounds good to me.

[satorux1, 2013-04-11 08:22:39]

this is obsolete right? closing...