Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2736683003:
Handle the case where Purge is called while upload is in progress. (Closed)
|
Created:
3 years, 9 months ago by Steven Holte Modified:
3 years, 9 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle the case where Purge is called while upload is in progress.
BUG=695433
Review-Url: https://codereview.chromium.org/2736683003
Cr-Commit-Position: refs/heads/master@{#456783}
Committed: https://chromium.googlesource.com/chromium/src/+/08f791ce023368b66e5412519025f9e16167a08d
Patch Set 1 #
Total comments: 6
Patch Set 2 : No purge histogram #Patch Set 3 : Update test #
Messages
Total messages: 19 (9 generated)
holte@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/2736683003/diff/1/components/ukm/ukm_service.cc File components/ukm/ukm_service.cc (left): https://codereview.chromium.org/2736683003/diff/1/components/ukm/ukm_service.... components/ukm/ukm_service.cc:313: persisted_logs_.DiscardStagedLog(); You'll need to rebase on my change. https://codereview.chromium.org/2736683003/diff/1/components/ukm/ukm_service.cc File components/ukm/ukm_service.cc (right): https://codereview.chromium.org/2736683003/diff/1/components/ukm/ukm_service.... components/ukm/ukm_service.cc:275: if (!persisted_logs_.has_staged_log()) One other thing I noticed is this logic. If StartScheduledUpload() is called while a log upload is in progress, the second oncomplete might not have the staged log - since we don't queue them up or anything. Now, maybe it's impossible - but for example see this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=669542 where this was happening for variations service when the process was suspended for a while. If it is possible to happen, we should perhaps have logic here - e.g. to not do the upload until the previous one finished. https://codereview.chromium.org/2736683003/diff/1/components/ukm/ukm_service.... components/ukm/ukm_service.cc:320: UMA_HISTOGRAM_SPARSE_SLOWLY("UKM.Upload.Purged.ResponseCode", I don't understand why we should do a separate code path and separate histogram when a log is purged. If it's a valid thing to happen, why not just update my TODO comment to explain the case and not do anything special?
https://codereview.chromium.org/2736683003/diff/1/components/ukm/ukm_service.cc File components/ukm/ukm_service.cc (left): https://codereview.chromium.org/2736683003/diff/1/components/ukm/ukm_service.... components/ukm/ukm_service.cc:313: persisted_logs_.DiscardStagedLog(); On 2017/03/07 22:02:10, Alexei Svitkine (very slow) wrote: > You'll need to rebase on my change. Done. https://codereview.chromium.org/2736683003/diff/1/components/ukm/ukm_service.cc File components/ukm/ukm_service.cc (right): https://codereview.chromium.org/2736683003/diff/1/components/ukm/ukm_service.... components/ukm/ukm_service.cc:275: if (!persisted_logs_.has_staged_log()) On 2017/03/07 22:02:10, Alexei Svitkine (very slow) wrote: > One other thing I noticed is this logic. > > If StartScheduledUpload() is called while a log upload is in progress, the > second oncomplete might not have the staged log - since we don't queue them up > or anything. > > Now, maybe it's impossible - but for example see this bug: > https://bugs.chromium.org/p/chromium/issues/detail?id=669542 > > where this was happening for variations service when the process was suspended > for a while. > > If it is possible to happen, we should perhaps have logic here - e.g. to not do > the upload until the previous one finished. > StartScheduledUpload should not be called while upload_in_progress is true since we wait scheduler_->UploadFinished to re-arm the timer. https://codereview.chromium.org/2736683003/diff/1/components/ukm/ukm_service.... components/ukm/ukm_service.cc:320: UMA_HISTOGRAM_SPARSE_SLOWLY("UKM.Upload.Purged.ResponseCode", On 2017/03/07 22:02:10, Alexei Svitkine (very slow) wrote: > I don't understand why we should do a separate code path and separate histogram > when a log is purged. > > If it's a valid thing to happen, why not just update my TODO comment to explain > the case and not do anything special? Was just adding this to record how often it actually happened. Removed it for now.
lgtm
The CQ bit was checked by holte@chromium.org
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by holte@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2736683003/#ps40001 (title: "Update test")
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by holte@chromium.org
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": 40001, "attempt_start_ts": 1489516439987070,
"parent_rev": "7858bc70062f09e1cd60e9a99dd8e5129d2c0ff2", "commit_rev":
"08f791ce023368b66e5412519025f9e16167a08d"}
Message was sent while issue was closed.
Description was changed from ========== Handle the case where Purge is called while upload is in progress. BUG=695433 ========== to ========== Handle the case where Purge is called while upload is in progress. BUG=695433 Review-Url: https://codereview.chromium.org/2736683003 Cr-Commit-Position: refs/heads/master@{#456783} Committed: https://chromium.googlesource.com/chromium/src/+/08f791ce023368b66e5412519025... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/08f791ce023368b66e5412519025... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
