Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1875443003:
Retry uploading in UploadJobImpl when error occurs (Closed)
|
Created:
4 years, 8 months ago by Marton Hunyady Modified:
4 years, 7 months ago Reviewers:
Andrew T Wilson (Slow) CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRetry uploading in UploadJobImpl when error occurs
UploadJobImpl retries uploading when errors (authorization, network
or server error) occurs up to a maximum of 3 retries, with a delay
between the attempts.
BUG=601730
Committed: https://crrev.com/2751d2194788416ca1949e51b29ed09fd1704c47
Cr-Commit-Position: refs/heads/master@{#394411}
Patch Set 1 #Patch Set 2 : Remove missing method #
Total comments: 6
Patch Set 3 : Minor comment changes #Patch Set 4 : Add delay between retries #Patch Set 5 : Retry when token request failed, add backoff, add log messages #
Total comments: 24
Patch Set 6 : Threading checks, logging, test improvements #
Total comments: 19
Patch Set 7 : Change testing, fixing smaller problems #Patch Set 8 : Remove unused include #
Total comments: 6
Patch Set 9 : Remove task_runner_ member from upload_job_unittest.cc #Patch Set 10 : Fix merge problem #Patch Set 11 : Fix include #
Messages
Total messages: 21 (8 generated)
hunyadym@chromium.org changed reviewers: + atwilson@chromium.org
Could you please review my code?
LGTM, once you reach out to cschuet@ and/or jinzhang@ around doing an actual test vs the real upload server. https://codereview.chromium.org/1875443003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_impl.cc (right): https://codereview.chromium.org/1875443003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:31: // Number of upload tries. nit: tries -> attempts https://codereview.chromium.org/1875443003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:312: // Request new token and retry nit: if your comment is a full sentence, you should put a period at the end (e.g. "Request new token and retry." https://codereview.chromium.org/1875443003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_impl.h (right): https://codereview.chromium.org/1875443003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.h:76: // ... -> PPREPARING_CONTENT -> UPLOADING -> ERROR nit: PPREPARING->PREPARING
https://codereview.chromium.org/1875443003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_impl.cc (right): https://codereview.chromium.org/1875443003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:31: // Number of upload tries. On 2016/04/08 15:42:56, Andrew T Wilson (Slow) wrote: > nit: tries -> attempts Done. https://codereview.chromium.org/1875443003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:312: // Request new token and retry On 2016/04/08 15:42:55, Andrew T Wilson (Slow) wrote: > nit: if your comment is a full sentence, you should put a period at the end > (e.g. "Request new token and retry." Done. https://codereview.chromium.org/1875443003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_impl.h (right): https://codereview.chromium.org/1875443003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.h:76: // ... -> PPREPARING_CONTENT -> UPLOADING -> ERROR On 2016/04/08 15:42:56, Andrew T Wilson (Slow) wrote: > nit: PPREPARING->PREPARING Done.
Main takeaway is I'd like us to be more explicit about what thread we expect each piece to be running on. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/remote_commands/screenshot_delegate.cc (right): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/remote_commands/screenshot_delegate.cc:64: base::ThreadTaskRunnerHandle::Get())); Why do we use this instead of blocking_task_runner_? https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:109: base::ThreadTaskRunnerHandle::Get())); Why not use task_runner_ here? If there's a reason, we should document it. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_impl.cc (left): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:295: access_token_request_.reset(); Why did you remove this? I think you still want to clear out the access_token_request_, even though it will be overwritten by RequestAccessToken. In fact, you may want to add a DCHECK_EQ(nullptr, access_token_request_) in RequestAccessToken() just to make sure we don't call it with an existing request already active. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_impl.cc (right): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:39: // the delay increases with this value. Consider just having a simple fixed 25s delay - the important thing is that we try a few times before the server times out, actually increasing the retry interval isn't important so I'd err towards simplicity. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:138: const scoped_refptr<base::SequencedTaskRunner> task_runner) pass as a reference https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:148: task_runner_(task_runner), Now that we have a task_runner, we need to make sure we're being called on the right thread. So I would recommend adding a base::ThreadChecker to this class, and calling thread_checker_.CalledOnValidThread() in every method that is invoked via the task_runner_ (as well as any public methods). https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:182: retry_ = 0; Why is this necessary? You can only get to IDLE state via the constructor, which sets retry_ to 0. If you are concerned that retry_ might not be 0 here, then add a DCHECK_EQ(0, retry_); https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:311: retry_++; Should log the errorCode here. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:355: // Successful uploading nit: uploading -> upload. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_impl.h (right): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.h:58: const scoped_refptr<base::SequencedTaskRunner> task_runner); Do you make any assumptions about this task_runner (for example, is it OK for tasks posted to that runner to be run from a different thread than the one the constructor is called on)? It's important to document this and also to add some DCHECKs to the code to make sure that assumption is maintained. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_unittest.cc (right): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_unittest.cc:155: class ImmediateThreadTaskRunner : public base::SingleThreadTaskRunner { Can you use TestSimpleTaskRunner instead? https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_unittest.cc:368: oauth2_service_.AddTokenToQueue(kTokenValid); Is there a test that ensures that we retry multiple times?
atwilson: PTAL https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/remote_commands/screenshot_delegate.cc (right): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/remote_commands/screenshot_delegate.cc:64: base::ThreadTaskRunnerHandle::Get())); On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > Why do we use this instead of blocking_task_runner_? blocking_task_runner_ is used to store the screenshot on the disk, so it belongs to the IO thread. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader.cc:109: base::ThreadTaskRunnerHandle::Get())); On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > Why not use task_runner_ here? If there's a reason, we should document it. Done. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_impl.cc (left): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:295: access_token_request_.reset(); On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > Why did you remove this? I think you still want to clear out the > access_token_request_, even though it will be overwritten by RequestAccessToken. > In fact, you may want to add a DCHECK_EQ(nullptr, access_token_request_) in > RequestAccessToken() just to make sure we don't call it with an existing request > already active. Done. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_impl.cc (right): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:39: // the delay increases with this value. On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > Consider just having a simple fixed 25s delay - the important thing is that we > try a few times before the server times out, actually increasing the retry > interval isn't important so I'd err towards simplicity. Done. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:138: const scoped_refptr<base::SequencedTaskRunner> task_runner) On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > pass as a reference Removed const, passing as value. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:148: task_runner_(task_runner), On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > Now that we have a task_runner, we need to make sure we're being called on the > right thread. So I would recommend adding a base::ThreadChecker to this class, > and calling thread_checker_.CalledOnValidThread() in every method that is > invoked via the task_runner_ (as well as any public methods). Done. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:182: retry_ = 0; On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > Why is this necessary? You can only get to IDLE state via the constructor, which > sets retry_ to 0. > > If you are concerned that retry_ might not be 0 here, then add a DCHECK_EQ(0, > retry_); Done. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:311: retry_++; On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > Should log the errorCode here. Done. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.cc:355: // Successful uploading On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > nit: uploading -> upload. Done. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_impl.h (right): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_impl.h:58: const scoped_refptr<base::SequencedTaskRunner> task_runner); On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > Do you make any assumptions about this task_runner (for example, is it OK for > tasks posted to that runner to be run from a different thread than the one the > constructor is called on)? It's important to document this and also to add some > DCHECKs to the code to make sure that assumption is maintained. Done. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/upload_job_unittest.cc (right): https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_unittest.cc:155: class ImmediateThreadTaskRunner : public base::SingleThreadTaskRunner { On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > Can you use TestSimpleTaskRunner instead? Unfortunately, I think no. Other possible solution may be to create a setter to the retry delay and set this before running the tests. https://codereview.chromium.org/1875443003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/upload_job_unittest.cc:368: oauth2_service_.AddTokenToQueue(kTokenValid); On 2016/05/02 09:15:49, Andrew T Wilson (Slow) wrote: > Is there a test that ensures that we retry multiple times? Done.
Description was changed from ========== Retry uploading in UploadJobImpl when error occurs UploadJobImpl retries uploading when errors (authorization, network or server error) occurs up to a maximum of 3 tries. BUG=601730 ========== to ========== Retry uploading in UploadJobImpl when error occurs UploadJobImpl retries uploading when errors (authorization, network or server error) occurs up to a maximum of 3 retries. BUG=601730 ==========
Description was changed from ========== Retry uploading in UploadJobImpl when error occurs UploadJobImpl retries uploading when errors (authorization, network or server error) occurs up to a maximum of 3 retries. BUG=601730 ========== to ========== Retry uploading in UploadJobImpl when error occurs UploadJobImpl retries uploading when errors (authorization, network or server error) occurs up to a maximum of 3 retries, with a delay between the attempts. BUG=601730 ==========
Description was changed from ========== Retry uploading in UploadJobImpl when error occurs UploadJobImpl retries uploading when errors (authorization, network or server error) occurs up to a maximum of 3 retries, with a delay between the attempts. BUG=601730 ========== to ========== Retry uploading in UploadJobImpl when error occurs UploadJobImpl retries uploading when errors (authorization, network or server error) occurs up to a maximum of 3 retries, with a delay between the attempts. BUG=601730 ==========
https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:15: #include "base/thread_task_runner_handle.h" Is this include necessary still? https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/upload_job_impl.cc (right): https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.cc:181: DCHECK(thread_checker_.CalledOnValidThread()); Move call to CalledOnValidThread() to the top of the function, in every place you use it in this file. It should be the first thing done in every function you call it from. https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.cc:316: void UploadJobImpl::HandleError(ErrorCode errorCode) { errorCode -> error_code https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.cc:320: std::string errorString; errorString->error_string https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.cc:332: LOG(ERROR) << "Upload failed: " << errorString; This is OK, but I think it's superior to just log the errorCode itself. https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/upload_job_impl.h (right): https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.h:13: #include "base/memory/ref_counted.h" #include "base/threading/thread_checker.h" (or whatever the right header file is for base::ThreadChecker https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.h:52: // |task_runner| must belong to the same thread from which the consturctor and nit: constructor https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/upload_job_unittest.cc (right): https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_unittest.cc:156: class ImmediateThreadTaskRunner : public base::SingleThreadTaskRunner { I still don't understand why this is required (why you can't use TestSimpleTaskRunner - maybe add a comment to this effect)? https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_unittest.cc:157: const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; What is the intended scope of this member variable? Maybe make it explicit (put it under public or protected or whatever) https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_unittest.cc:358: class UploadServerErrorTest : public UploadJobTestBase { Could you use UploadFlowTest() but have some member variable that makes HandlePostRequest return INTERNAL_SERVER_ERROR, rather than making another test class? This is fine as-is, although it does have some code duplication between the two classes it might be nice to avoid.
https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:15: #include "base/thread_task_runner_handle.h" On 2016/05/10 22:00:42, Andrew T Wilson (Slow) wrote: > Is this include necessary still? Done. https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/upload_job_impl.cc (right): https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.cc:181: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/05/10 22:00:42, Andrew T Wilson (Slow) wrote: > Move call to CalledOnValidThread() to the top of the function, in every place > you use it in this file. It should be the first thing done in every function you > call it from. Done. https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.cc:316: void UploadJobImpl::HandleError(ErrorCode errorCode) { On 2016/05/10 22:00:42, Andrew T Wilson (Slow) wrote: > errorCode -> error_code Done. https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.cc:320: std::string errorString; On 2016/05/10 22:00:42, Andrew T Wilson (Slow) wrote: > errorString->error_string Done. https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.cc:332: LOG(ERROR) << "Upload failed: " << errorString; On 2016/05/10 22:00:42, Andrew T Wilson (Slow) wrote: > This is OK, but I think it's superior to just log the errorCode itself. Done. https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/upload_job_impl.h (right): https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.h:13: #include "base/memory/ref_counted.h" On 2016/05/10 22:00:42, Andrew T Wilson (Slow) wrote: > #include "base/threading/thread_checker.h" (or whatever the right header file is > for base::ThreadChecker Done. https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_impl.h:52: // |task_runner| must belong to the same thread from which the consturctor and On 2016/05/10 22:00:42, Andrew T Wilson (Slow) wrote: > nit: constructor Done. https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/upload_job_unittest.cc (right): https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_unittest.cc:156: class ImmediateThreadTaskRunner : public base::SingleThreadTaskRunner { On 2016/05/10 22:00:42, Andrew T Wilson (Slow) wrote: > I still don't understand why this is required (why you can't use > TestSimpleTaskRunner - maybe add a comment to this effect)? I removed it and made it possible to set the delays for the tests separately, so now I'm using base::ThreadTaskRunnerHandle::Get() as task runner for the tests. The problem with TestSimpleTaskRunner is that I didn't find a way to run the tasks posted to that automatically until run_loop_.Quit() is called from the callbacks. https://codereview.chromium.org/1875443003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_unittest.cc:358: class UploadServerErrorTest : public UploadJobTestBase { On 2016/05/10 22:00:42, Andrew T Wilson (Slow) wrote: > Could you use UploadFlowTest() but have some member variable that makes > HandlePostRequest return INTERNAL_SERVER_ERROR, rather than making another test > class? > > This is fine as-is, although it does have some code duplication between the two > classes it might be nice to avoid. Done, removed it.
Still LGTM, but would suggest fixing upload_job_unittest.cc to remove task_runner_ member variable. https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:114: task_runner_)); This line I think is mis-indented. https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/upload_job_unittest.cc (right): https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_unittest.cc:227: scoped_refptr<base::SequencedTaskRunner> task_runner_; Why do we need to track this in a member variable? Can this just be a local variable in PrepareUploadJob(), since it's a refptr and UploadJobImpl should hold its own reference (no need to keep it around in a member var)? https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_unittest.cc:347: // kMaxAttempts This is fine. Alternative would have been to expose this in the UploadJob header file, but I think this is OK.
https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:114: task_runner_)); On 2016/05/18 11:59:38, Andrew T Wilson (Slow) wrote: > This line I think is mis-indented. task_runner_ belongs to the same level as everything above it (constructor parameter of UploadJobImpl), so I think it's OK. https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/upload_job_unittest.cc (right): https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/upload_job_unittest.cc:227: scoped_refptr<base::SequencedTaskRunner> task_runner_; On 2016/05/18 11:59:38, Andrew T Wilson (Slow) wrote: > Why do we need to track this in a member variable? Can this just be a local > variable in PrepareUploadJob(), since it's a refptr and UploadJobImpl should > hold its own reference (no need to keep it around in a member var)? Done.
https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader.cc (right): https://codereview.chromium.org/1875443003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader.cc:114: task_runner_)); On 2016/05/18 12:27:03, Marton wrote: > On 2016/05/18 11:59:38, Andrew T Wilson (Slow) wrote: > > This line I think is mis-indented. > > task_runner_ belongs to the same level as everything above it (constructor > parameter of UploadJobImpl), so I think it's OK. You are right, I misread it :)
The CQ bit was checked by hunyadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/1875443003/#ps200001 (title: "Fix include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1875443003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1875443003/200001
Message was sent while issue was closed.
Description was changed from ========== Retry uploading in UploadJobImpl when error occurs UploadJobImpl retries uploading when errors (authorization, network or server error) occurs up to a maximum of 3 retries, with a delay between the attempts. BUG=601730 ========== to ========== Retry uploading in UploadJobImpl when error occurs UploadJobImpl retries uploading when errors (authorization, network or server error) occurs up to a maximum of 3 retries, with a delay between the attempts. BUG=601730 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Retry uploading in UploadJobImpl when error occurs UploadJobImpl retries uploading when errors (authorization, network or server error) occurs up to a maximum of 3 retries, with a delay between the attempts. BUG=601730 ========== to ========== Retry uploading in UploadJobImpl when error occurs UploadJobImpl retries uploading when errors (authorization, network or server error) occurs up to a maximum of 3 retries, with a delay between the attempts. BUG=601730 Committed: https://crrev.com/2751d2194788416ca1949e51b29ed09fd1704c47 Cr-Commit-Position: refs/heads/master@{#394411} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2751d2194788416ca1949e51b29ed09fd1704c47 Cr-Commit-Position: refs/heads/master@{#394411} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
