Parallel Download finch config parameters on Chrome client.

Parallel Download finch config parameters on Chrome client.

Read several config from finch for parallel download.

We currently didn't add the server side json config file yet.

The parameter key on the server config should match the client side
constants.

BUG=644352
Review-Url: https://codereview.chromium.org/2730363004
Cr-Commit-Position: refs/heads/master@{#455374}
Committed: https://chromium.googlesource.com/chromium/src/+/696a6d27f072467ab1722a8b61b114ebb1f8d595

[xingliu, 2017-03-06 20:43:21]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[xingliu, 2017-03-06 20:43:34]

xingliu@chromium.org changed reviewers:
+ qinmin@chromium.org

[xingliu, 2017-03-06 20:43:35]

Hi, PTAL, thanks.

[xingliu, 2017-03-06 20:44:06]

xingliu@chromium.org changed reviewers:
+ dtrainor@chromium.org

[xingliu, 2017-03-06 20:44:07]

Hi, PTAL, thanks.

[commit-bot: I haz the power, 2017-03-06 20:44:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qinmin, 2017-03-06 22:29:08]

https://codereview.chromium.org/2730363004/diff/20001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2730363004/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:18: const char
kMinFileSizeFinchKey[] = "min_file_size";
shouldn't be file size, it could be part of a slice.

For example, a file is 1G bytes large, and we use 2 workers to download it. If
the first worker downloaded 100 MB and chrome crashes right before the 2nd
worker is able to download any data, then we should still fork the 2nd worker
when resuming the download 

I would use MinSliceSize instead

[commit-bot: I haz the power, 2017-03-06 22:38:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 22:38:28]

Dry run: This issue passed the CQ dry run.

[xingliu, 2017-03-06 23:30:21]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 23:30:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2017-03-06 23:37:05]

Thanks for the review, please take another look.

https://codereview.chromium.org/2730363004/diff/20001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2730363004/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:18: const char
kMinFileSizeFinchKey[] = "min_file_size";
On 2017/03/06 22:29:08, qinmin wrote:
> shouldn't be file size, it could be part of a slice.
> 
> For example, a file is 1G bytes large, and we use 2 workers to download it. If
> the first worker downloaded 100 MB and chrome crashes right before the 2nd
> worker is able to download any data, then we should still fork the 2nd worker
> when resuming the download 
> 
> I would use MinSliceSize instead

Done. Also polished some comments.

One question of the expected behavior: If we have 2 holes and they are both
larger than MinSliceSize, and we can have at most 2 requests.

Then the 2 worker will download the first hole, then if one of them is finished,
we chunk the second hole, and start one new request on it?

[commit-bot: I haz the power, 2017-03-07 00:34:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 00:34:59]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[qinmin, 2017-03-07 05:53:26]

On 2017/03/06 23:37:05, xingliu wrote:
> Thanks for the review, please take another look.
> 
>
https://codereview.chromium.org/2730363004/diff/20001/content/browser/downloa...
> File content/browser/download/parallel_download_utils.cc (right):
> 
>
https://codereview.chromium.org/2730363004/diff/20001/content/browser/downloa...
> content/browser/download/parallel_download_utils.cc:18: const char
> kMinFileSizeFinchKey[] = "min_file_size";
> On 2017/03/06 22:29:08, qinmin wrote:
> > shouldn't be file size, it could be part of a slice.
> > 
> > For example, a file is 1G bytes large, and we use 2 workers to download it.
If
> > the first worker downloaded 100 MB and chrome crashes right before the 2nd
> > worker is able to download any data, then we should still fork the 2nd
worker
> > when resuming the download 
> > 
> > I would use MinSliceSize instead
> 
> Done. Also polished some comments.
> 
> One question of the expected behavior: If we have 2 holes and they are both
> larger than MinSliceSize, and we can have at most 2 requests.
> 
> Then the 2 worker will download the first hole, then if one of them is
finished,
> we chunk the second hole, and start one new request on it?

For simplicity, we can fork new requests only when DownloaditemImpl::Start() is
called.
After that, Chrome will wait for all the requests to finish without creating new
requests if one worker finishes.
Download can still get interrupted. But when it resumes, we can fork more than 1
requests at that time since DownloadItemImpl::Start() will be called again.

[qinmin, 2017-03-07 05:56:50]

lgtm % nit

https://codereview.chromium.org/2730363004/diff/60001/content/browser/downloa...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2730363004/diff/60001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:18:
request_num_(GetParallelRequestCountConfig()) {}
nit: this variable is no longer needed, just call
GetParallelRequestCountConfig() in the code should be fine

[xingliu, 2017-03-07 18:44:08]

Thanks for the review.

https://codereview.chromium.org/2730363004/diff/20001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2730363004/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:18: const char
kMinFileSizeFinchKey[] = "min_file_size";
On 2017/03/06 22:29:08, qinmin wrote:
> shouldn't be file size, it could be part of a slice.
> 
> For example, a file is 1G bytes large, and we use 2 workers to download it. If
> the first worker downloaded 100 MB and chrome crashes right before the 2nd
> worker is able to download any data, then we should still fork the 2nd worker
> when resuming the download 
> 
> I would use MinSliceSize instead

Got it.

https://codereview.chromium.org/2730363004/diff/60001/content/browser/downloa...
File content/browser/download/parallel_download_job.cc (right):

https://codereview.chromium.org/2730363004/diff/60001/content/browser/downloa...
content/browser/download/parallel_download_job.cc:18:
request_num_(GetParallelRequestCountConfig()) {}
On 2017/03/07 05:56:50, qinmin wrote:
> nit: this variable is no longer needed, just call
> GetParallelRequestCountConfig() in the code should be fine

Done, makes sense.

[xingliu, 2017-03-07 18:44:17]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 18:45:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 19:05:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 19:05:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xingliu, 2017-03-07 19:26:34]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 19:27:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 21:18:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 21:18:33]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xingliu, 2017-03-08 01:26:40]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 01:27:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 02:29:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 02:29:53]

Dry run: This issue passed the CQ dry run.

[xingliu, 2017-03-08 02:31:50]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2017-03-08 02:31:51]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2730363004/#ps130001
(title: "Fix for new code merged in.")

[commit-bot: I haz the power, 2017-03-08 02:32:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 02:36:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 02:36:35]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xingliu, 2017-03-08 03:28:16]

The CQ bit was checked by xingliu@chromium.org

[commit-bot: I haz the power, 2017-03-08 03:28:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 03:36:31]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1488943696891430,
"parent_rev": "0d9346b1e64f41e97cd44a291ae43343bc0edcc7", "commit_rev":
"696a6d27f072467ab1722a8b61b114ebb1f8d595"}

[commit-bot: I haz the power, 2017-03-08 03:37:10]

Description was changed from

==========
Parallel Download finch config parameters on Chrome client.

Read several config from finch for parallel download.

We currently didn't add the server side json config file yet.

The parameter key on the server config should match the client side
constants.

BUG=644352
==========

to

==========
Parallel Download finch config parameters on Chrome client.

Read several config from finch for parallel download.

We currently didn't add the server side json config file yet.

The parameter key on the server config should match the client side
constants.

BUG=644352

Review-Url: https://codereview.chromium.org/2730363004
Cr-Commit-Position: refs/heads/master@{#455374}
Committed:
https://chromium.googlesource.com/chromium/src/+/696a6d27f072467ab1722a8b61b1...
==========

[commit-bot: I haz the power, 2017-03-08 03:37:12]

Committed patchset #8 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/696a6d27f072467ab1722a8b61b1...