Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750853004:
Add a delay to send the parallel requests. (Closed)
DescriptionAdd a delay to send the parallel requests.
After the response of the original request, and a delay, parallel
requests are sent.
The delay is finch configurable, and default value is 0, so unit tests
don't take the delay.
BUG=644352
Review-Url: https://codereview.chromium.org/2750853004
Cr-Commit-Position: refs/heads/master@{#457564}
Committed: https://chromium.googlesource.com/chromium/src/+/a217ace927455448460006a47fc00331dd4ca30a
Patch Set 1 #
Total comments: 6
Patch Set 2 : Work on feedback. #Patch Set 3 : Polish the condition check, #
Total comments: 14
Patch Set 4 : Work on feedback. #
Total comments: 2
Patch Set 5 : Work on nits. #
Messages
Total messages: 23 (9 generated)
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, qinmin@chromium.org
Hi, PTAL, thanks.
lgtm https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... File content/browser/download/parallel_download_utils.cc (right): https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... content/browser/download/parallel_download_utils.cc:120: return !finch_value.empty() && base::StringToInt64(finch_value, &time_ms) base::StringToInt64 will return false if you pass an empty string.
https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... File content/browser/download/parallel_download_job.h (right): https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... content/browser/download/parallel_download_job.h:65: std::unique_ptr<base::OneShotTimer> timer_; You don't need the unique_ptr, just use base::OneShotTimer timer_. https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... File content/browser/download/parallel_download_utils.cc (right): https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... content/browser/download/parallel_download_utils.cc:37: const base::TimeDelta kParallelRequestDelay = move this inside GetParallelRequestDelayConfig(),
Thanks for the feedback, polished some details. https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... File content/browser/download/parallel_download_job.h (right): https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... content/browser/download/parallel_download_job.h:65: std::unique_ptr<base::OneShotTimer> timer_; On 2017/03/15 05:49:18, qinmin wrote: > You don't need the unique_ptr, just use base::OneShotTimer timer_. Done, make sense. https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... File content/browser/download/parallel_download_utils.cc (right): https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... content/browser/download/parallel_download_utils.cc:37: const base::TimeDelta kParallelRequestDelay = On 2017/03/15 05:49:18, qinmin wrote: > move this inside GetParallelRequestDelayConfig(), Done. https://codereview.chromium.org/2750853004/diff/1/content/browser/download/pa... content/browser/download/parallel_download_utils.cc:120: return !finch_value.empty() && base::StringToInt64(finch_value, &time_ms) On 2017/03/15 03:47:38, David Trainor-ping if over 24h wrote: > base::StringToInt64 will return false if you pass an empty string. Done, oh, i see, removed !finch_value.empty().
https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:33: DownloadJobImpl::Cancel(user_cancel); you need to stop the timer_ here https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:39: DownloadJobImpl::Pause(); need some book keeping here. If the timer is active, record its state, and stop the timer https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:45: DownloadJobImpl::Resume(resume_request); Need to start the timer again if it is paused previously.
https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:80: FindSlicesToDownload(download_item_->GetReceivedSlices()); There is also an issue here. The initial request might have write some data before the delayed task is executed. And the updated ReceivedSlices information is not yet reported to DownloadItem as the write/report happens on different thread.
Some discussion. https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:33: DownloadJobImpl::Cancel(user_cancel); On 2017/03/15 18:32:38, qinmin wrote: > you need to stop the timer_ here Discussed below. https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:39: DownloadJobImpl::Pause(); On 2017/03/15 18:32:38, qinmin wrote: > need some book keeping here. If the timer is active, record its state, and stop > the timer Yeah, we need state control in this class. Can we do it in next CL? Also probably directly control the request handle. https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:45: DownloadJobImpl::Resume(resume_request); On 2017/03/15 18:32:38, qinmin wrote: > Need to start the timer again if it is paused previously. Shall we use requests handle to control the state, wdyt? Like we send the requests and get response, if they are paused, we pause the request handle instead of the timer. Since pause the timer will introduce another state control. https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:80: FindSlicesToDownload(download_item_->GetReceivedSlices()); On 2017/03/15 19:09:00, qinmin wrote: > There is also an issue here. The initial request might have write some data > before the delayed task is executed. And the updated ReceivedSlices information > is not yet reported to DownloadItem as the write/report happens on different > thread. This is probably OK, if DownloadFile is smart enough to discard later streams or continue writing.
https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:39: DownloadJobImpl::Pause(); On 2017/03/15 19:37:11, xingliu wrote: > On 2017/03/15 18:32:38, qinmin wrote: > > need some book keeping here. If the timer is active, record its state, and > stop > > the timer > > Yeah, we need state control in this class. Can we do it in next CL? Also > probably directly control the request handle. This is different from the request handle. You don't even need to create the request handles in this case. https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:45: DownloadJobImpl::Resume(resume_request); On 2017/03/15 19:37:11, xingliu wrote: > On 2017/03/15 18:32:38, qinmin wrote: > > Need to start the timer again if it is paused previously. > > Shall we use requests handle to control the state, wdyt? > > Like we send the requests and get response, if they are paused, we pause the > request handle instead of the timer. > > Since pause the timer will introduce another state control. The timer is a one-time thing before the requests are created. So if user has already paused a download, why should we create those request handle?
https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:33: DownloadJobImpl::Cancel(user_cancel); On 2017/03/15 19:37:11, xingliu wrote: > On 2017/03/15 18:32:38, qinmin wrote: > > you need to stop the timer_ here > > Discussed below. Nice catch Min!
Thanks for the feedback, PTAL. https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:33: DownloadJobImpl::Cancel(user_cancel); On 2017/03/15 22:39:54, David Trainor-ping if over 24h wrote: > On 2017/03/15 19:37:11, xingliu wrote: > > On 2017/03/15 18:32:38, qinmin wrote: > > > you need to stop the timer_ here > > > > Discussed below. > > Nice catch Min! Done. https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:39: DownloadJobImpl::Pause(); On 2017/03/15 21:50:57, qinmin wrote: > On 2017/03/15 19:37:11, xingliu wrote: > > On 2017/03/15 18:32:38, qinmin wrote: > > > need some book keeping here. If the timer is active, record its state, and > > stop > > > the timer > > > > Yeah, we need state control in this class. Can we do it in next CL? Also > > probably directly control the request handle. > > This is different from the request handle. You don't even need to create the > request handles in this case. Makes sense, done. This is better. https://codereview.chromium.org/2750853004/diff/2/content/browser/download/pa... content/browser/download/parallel_download_job.cc:45: DownloadJobImpl::Resume(resume_request); On 2017/03/15 21:50:57, qinmin wrote: > On 2017/03/15 19:37:11, xingliu wrote: > > On 2017/03/15 18:32:38, qinmin wrote: > > > Need to start the timer again if it is paused previously. > > > > Shall we use requests handle to control the state, wdyt? > > > > Like we send the requests and get response, if they are paused, we pause the > > request handle instead of the timer. > > > > Since pause the timer will introduce another state control. > > The timer is a one-time thing before the requests are created. So if user has > already paused a download, why should we create those request handle? Done.
lgtm%nit https://codereview.chromium.org/2750853004/diff/50001/content/browser/downloa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2750853004/diff/50001/content/browser/downloa... content/browser/download/parallel_download_job.cc:106: if (requests_sent_) nit: DCHECK_FALSE(request_sent);
Thanks for the review. Polished the DCHECK. https://codereview.chromium.org/2750853004/diff/50001/content/browser/downloa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2750853004/diff/50001/content/browser/downloa... content/browser/download/parallel_download_job.cc:106: if (requests_sent_) On 2017/03/16 17:40:36, qinmin wrote: > nit: DCHECK_FALSE(request_sent); Done.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/2750853004/#ps70001 (title: "Work on nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 70001, "attempt_start_ts": 1489699449036410,
"parent_rev": "eb260ae77f464ec4b53627278b9e842cafb3490e", "commit_rev":
"a217ace927455448460006a47fc00331dd4ca30a"}
Message was sent while issue was closed.
Description was changed from ========== Add a delay to send the parallel requests. After the response of the original request, and a delay, parallel requests are sent. The delay is finch configurable, and default value is 0, so unit tests don't take the delay. BUG=644352 ========== to ========== Add a delay to send the parallel requests. After the response of the original request, and a delay, parallel requests are sent. The delay is finch configurable, and default value is 0, so unit tests don't take the delay. BUG=644352 Review-Url: https://codereview.chromium.org/2750853004 Cr-Commit-Position: refs/heads/master@{#457564} Committed: https://chromium.googlesource.com/chromium/src/+/a217ace927455448460006a47fc0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001) as https://chromium.googlesource.com/chromium/src/+/a217ace927455448460006a47fc0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
