Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2112483002:
sandwich: Fixes two sources of KeyError task failures (Closed)
|
Created:
4 years, 5 months ago by gabadie Modified:
4 years, 5 months ago Reviewers:
pasko CC:
chromium-reviews, mikecase+watch_chromium.org, gabadie+watch_chromium.org, jbudorick+watch_chromium.org, lizeb+watch-android-loading_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionsandwich: Fixes two sources of KeyError task failures
BUG=623966
Committed: https://crrev.com/66cf3abbaa67cfa8e9f6707b4da1d95d79bd7879
Cr-Commit-Position: refs/heads/master@{#403916}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase #Patch Set 3 : Addresses Egor's comments #
Total comments: 8
Patch Set 4 : Addresses egor's comments #
Total comments: 2
Patch Set 5 : Address pasko #Patch Set 6 : Addresses Egor's comments #Messages
Total messages: 14 (3 generated)
gabadie@chromium.org changed reviewers: + pasko@chromium.org
overall looks good, a few small suggestions https://codereview.chromium.org/2112483002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_prefetch.py (right): https://codereview.chromium.org/2112483002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_prefetch.py:205: # TODO(gabadie): Dig why these requests were not in WPR. nit: s/Dig/Investigate/ yeah, that's a bit surprising, and suggests to me that our scanner discoverer is more magic than it should be :( https://codereview.chromium.org/2112483002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_prefetch.py:504: # load. can you spit a log message here? It would be nice to make a list of URLs that overflow memory cache - for folks who are busy reimplementing the memorycache. https://codereview.chromium.org/2112483002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_prefetch.py:598: # in the WPR archive? please link to the bug (and preferably the comment # in the bug) here that lists the URL that can trigger this. ... otherwise this TODO will be harder than necessary to address in the future.
https://codereview.chromium.org/2112483002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_prefetch.py (right): https://codereview.chromium.org/2112483002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_prefetch.py:205: # TODO(gabadie): Dig why these requests were not in WPR. On 2016/07/04 15:55:19, pasko wrote: > nit: s/Dig/Investigate/ > > yeah, that's a bit surprising, and suggests to me that our scanner discoverer is > more magic than it should be :( Done. https://codereview.chromium.org/2112483002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_prefetch.py:504: # load. On 2016/07/04 15:55:19, pasko wrote: > can you spit a log message here? It would be nice to make a list of URLs that > overflow memory cache - for folks who are busy reimplementing the memorycache. Done. https://codereview.chromium.org/2112483002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_prefetch.py:598: # in the WPR archive? On 2016/07/04 15:55:18, pasko wrote: > please link to the bug (and preferably the comment # in the bug) here that lists > the URL that can trigger this. > > ... otherwise this TODO will be harder than necessary to address in the future. Done.
https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... File tools/android/loading/sandwich_prefetch.py (right): https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... tools/android/loading/sandwich_prefetch.py:602: # in the WPR archive? crbug.com/623966#c5 Example URL can be found in: http://crbug.com/623966#c5 https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... File tools/android/loading/sandwich_utils.py (right): https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... tools/android/loading/sandwich_utils.py:18: def NormalizeUrl(url): better inline this function. Called once and the name is quite cryptic making an extra file hop for the reader https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... tools/android/loading/sandwich_utils.py:21: parsed_url[2] = re.sub(r"/{2,}", r"/", parsed_url[2]) single quotes for consistency
https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... File tools/android/loading/sandwich_prefetch.py (right): https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... tools/android/loading/sandwich_prefetch.py:602: # in the WPR archive? crbug.com/623966#c5 On 2016/07/04 18:05:57, pasko wrote: > Example URL can be found in: http://crbug.com/623966#c5 Done. https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... File tools/android/loading/sandwich_utils.py (right): https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... tools/android/loading/sandwich_utils.py:18: def NormalizeUrl(url): On 2016/07/04 18:05:58, pasko wrote: > better inline this function. Called once and the name is quite cryptic making an > extra file hop for the reader I don't think so because it will very probably be needed for SWR has well, because there is a true issue that needs to be investigated between the different place were we already have normalized URLs, and where we do need to normalize them. https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... tools/android/loading/sandwich_utils.py:21: parsed_url[2] = re.sub(r"/{2,}", r"/", parsed_url[2]) On 2016/07/04 18:05:57, pasko wrote: > single quotes for consistency Oups... done.
https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... File tools/android/loading/sandwich_utils.py (right): https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... tools/android/loading/sandwich_utils.py:18: def NormalizeUrl(url): On 2016/07/06 08:57:55, gabadie wrote: > On 2016/07/04 18:05:58, pasko wrote: > > better inline this function. Called once and the name is quite cryptic making > an > > extra file hop for the reader > > I don't think so because it will very probably be needed for SWR has well, > because there is a true issue that needs to be investigated between the > different place were we already have normalized URLs, and where we do need to > normalize them. Let's not complicate reading before it is necessary. Once it is useful for SWR as well, it will be easy to extract these 3 lines into a common place. https://codereview.chromium.org/2112483002/diff/60001/tools/android/loading/s... File tools/android/loading/sandwich_utils.py (right): https://codereview.chromium.org/2112483002/diff/60001/tools/android/loading/s... tools/android/loading/sandwich_utils.py:14: class SandwichKnownError(Exception): We are not catching this anywhere, so what is the point of having another class? If one sees 'raise Exception(...)', the idea is clear, however, when there is 'raise SandwichKnownError(...)' it causes question like: "Is this exception handled in a special way somewhere?", "When should this be used to replace the regular exception?", etc. If the only purpose is to say 'Known Error' somewhere, you can do it in the message.
https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... File tools/android/loading/sandwich_utils.py (right): https://codereview.chromium.org/2112483002/diff/40001/tools/android/loading/s... tools/android/loading/sandwich_utils.py:18: def NormalizeUrl(url): On 2016/07/06 12:45:52, pasko wrote: > On 2016/07/06 08:57:55, gabadie wrote: > > On 2016/07/04 18:05:58, pasko wrote: > > > better inline this function. Called once and the name is quite cryptic > making > > an > > > extra file hop for the reader > > > > I don't think so because it will very probably be needed for SWR has well, > > because there is a true issue that needs to be investigated between the > > different place were we already have normalized URLs, and where we do need to > > normalize them. > > Let's not complicate reading before it is necessary. Once it is useful for SWR > as well, it will be easy to extract these 3 lines into a common place. Done. https://codereview.chromium.org/2112483002/diff/60001/tools/android/loading/s... File tools/android/loading/sandwich_utils.py (right): https://codereview.chromium.org/2112483002/diff/60001/tools/android/loading/s... tools/android/loading/sandwich_utils.py:14: class SandwichKnownError(Exception): On 2016/07/06 12:45:52, pasko wrote: > We are not catching this anywhere, so what is the point of having another class? > > If one sees 'raise Exception(...)', the idea is clear, however, when there is > 'raise SandwichKnownError(...)' it causes question like: "Is this exception > handled in a special way somewhere?", "When should this be used to replace the > regular exception?", etc. > > If the only purpose is to say 'Known Error' somewhere, you can do it in the > message. Done.
lgtm, thank you!
The CQ bit was checked by gabadie@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== sandwich: Fixes two sources of KeyError task failures BUG=623966 ========== to ========== sandwich: Fixes two sources of KeyError task failures BUG=623966 Committed: https://crrev.com/66cf3abbaa67cfa8e9f6707b4da1d95d79bd7879 Cr-Commit-Position: refs/heads/master@{#403916} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/66cf3abbaa67cfa8e9f6707b4da1d95d79bd7879 Cr-Commit-Position: refs/heads/master@{#403916} |
