Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2835253002:
Treat already existing saved pages as success. (Closed)
|
Created:
3 years, 8 months ago by Pete Williamson Modified:
3 years, 8 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTreat already existing saved pages as success.
In some cases, we ran into a failure mode where we saved off a copy of
a page, but then failed. When we retried, we were getting a result of
ALREADY_EXISTS for the page, but we treated it as an error, which
meant we could never succeed for that page. We now treat ALREADY_EXISTS
as a success case. Added a new unit test to check the new code.
BUG=704726
Review-Url: https://codereview.chromium.org/2835253002
Cr-Commit-Position: refs/heads/master@{#467025}
Committed: https://chromium.googlesource.com/chromium/src/+/85242f08db71684cbba4bcf94cce92ed91d9726f
Patch Set 1 #
Total comments: 2
Patch Set 2 : CR feedback per FGorski #Patch Set 3 : Fix build error #
Messages
Total messages: 36 (23 generated)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
petewil@chromium.org changed reviewers: + chili@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm but you might need a committer to take a look :D
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Tread already existing saved pages as success. In some cases, we ran into a failure mode where we saved off a copy of a page, but then failed. When we retried, we were getting a result of ALREADY_EXISTS for the page, but we treated it as an error, which meant we could never succeed for that page. We now treat ALREADY_EXISTS as a success case. Added a new unit test to check the new code. BUG=704726 ========== to ========== Tread already existing saved pages as success. In some cases, we ran into a failure mode where we saved off a copy of a page, but then failed. When we retried, we were getting a result of ALREADY_EXISTS for the page, but we treated it as an error, which meant we could never succeed for that page. We now treat ALREADY_EXISTS as a success case. Added a new unit test to check the new code. BUG=704726 ==========
The CQ bit was checked by petewil@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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== Tread already existing saved pages as success. In some cases, we ran into a failure mode where we saved off a copy of a page, but then failed. When we retried, we were getting a result of ALREADY_EXISTS for the page, but we treated it as an error, which meant we could never succeed for that page. We now treat ALREADY_EXISTS as a success case. Added a new unit test to check the new code. BUG=704726 ========== to ========== Treat already existing saved pages as success. In some cases, we ran into a failure mode where we saved off a copy of a page, but then failed. When we retried, we were getting a result of ALREADY_EXISTS for the page, but we treated it as an error, which meant we could never succeed for that page. We now treat ALREADY_EXISTS as a success case. Added a new unit test to check the new code. BUG=704726 ==========
petewil@chromium.org changed reviewers: + romax@chromium.org
Adding Romax for committer review.
lgtm
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/2835253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2835253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:368: save_result == SavePageResult::ALREADY_EXISTS) { Hey, 3 questions: 1. Did you check if we maybe remove the file if we get already_exists. 2. Also if it already exists, why would you consider it "on last retry" below? 3. What about a corresponding change on Prerendering offliner side to keep the expectation in line?
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
https://codereview.chromium.org/2835253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2835253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:368: save_result == SavePageResult::ALREADY_EXISTS) { On 2017/04/24 19:51:51, fgorski wrote: > Hey, 3 questions: > 1. Did you check if we maybe remove the file if we get already_exists. > We don't remove it, because we then get the same error on the next try. > 2. Also if it already exists, why would you consider it "on last retry" below? > It's a bit ambiguous, but I changed the code to not count it as a last retry. > 3. What about a corresponding change on Prerendering offliner side to keep the > expectation in line? Done (but no unit test added).
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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by petewil@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.
On 2017/04/25 00:09:00, Pete Williamson wrote: > https://codereview.chromium.org/2835253002/diff/1/chrome/browser/android/offl... > File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): > > https://codereview.chromium.org/2835253002/diff/1/chrome/browser/android/offl... > chrome/browser/android/offline_pages/background_loader_offliner.cc:368: > save_result == SavePageResult::ALREADY_EXISTS) { > On 2017/04/24 19:51:51, fgorski wrote: > > Hey, 3 questions: > > 1. Did you check if we maybe remove the file if we get already_exists. > > > We don't remove it, because we then get the same error on the next try. What I was referring to was deleting the archive file here: https://cs.chromium.org/chromium/src/components/offline_pages/core/offline_pa... It is not affecting the old copy thou, because we generate a random GUID per file name here: https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/off... > > 2. Also if it already exists, why would you consider it "on last retry" below? > > > It's a bit ambiguous, but I changed the code to not count it as a last retry. > > 3. What about a corresponding change on Prerendering offliner side to keep the > > expectation in line? > Done (but no unit test added).
lgtm, Would adding tests to prerendering offliner be hard thou?
On 2017/04/25 16:43:37, fgorski wrote: > lgtm, > > Would adding tests to prerendering offliner be hard thou? Per offline disussion, I added a new bug (crbug.com/71152) to track creating the new unit test. It is currently blocked by crbug.com/712941.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chili@chromium.org, romax@chromium.org Link to the patchset: https://codereview.chromium.org/2835253002/#ps40001 (title: "Fix build error")
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": 1493139862123140,
"parent_rev": "ed44ef1f1961716727c328153fd34b97fe8f9642", "commit_rev":
"85242f08db71684cbba4bcf94cce92ed91d9726f"}
Message was sent while issue was closed.
Description was changed from ========== Treat already existing saved pages as success. In some cases, we ran into a failure mode where we saved off a copy of a page, but then failed. When we retried, we were getting a result of ALREADY_EXISTS for the page, but we treated it as an error, which meant we could never succeed for that page. We now treat ALREADY_EXISTS as a success case. Added a new unit test to check the new code. BUG=704726 ========== to ========== Treat already existing saved pages as success. In some cases, we ran into a failure mode where we saved off a copy of a page, but then failed. When we retried, we were getting a result of ALREADY_EXISTS for the page, but we treated it as an error, which meant we could never succeed for that page. We now treat ALREADY_EXISTS as a success case. Added a new unit test to check the new code. BUG=704726 Review-Url: https://codereview.chromium.org/2835253002 Cr-Commit-Position: refs/heads/master@{#467025} Committed: https://chromium.googlesource.com/chromium/src/+/85242f08db71684cbba4bcf94cce... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/85242f08db71684cbba4bcf94cce... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
