Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
618083003:
QUIC - track the open and close states of disk cache entries of (Closed)
|
Created:
6 years, 2 months ago by ramant (doing other things) Modified:
6 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, mef Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionQUIC - track the open and close states of disk cache entries of
QuicServerInfo.
BUG=417835
R=rch@chromium.org, asvitkine@chromium.org
Committed: https://crrev.com/ddd25bc416a00c5cfd4a2b5051e514671088f639
Cr-Commit-Position: refs/heads/master@{#297776}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Minor change to QuicDiskCacheEntryState values #
Total comments: 6
Patch Set 3 : updated the doc per comments in Patch Set 2 #
Total comments: 4
Patch Set 4 : Fixed nits in Patch Set 3 #Patch Set 5 : Merge with TOT #
Messages
Total messages: 23 (9 generated)
Patchset #1 (id:1) has been deleted
lgtm https://codereview.chromium.org/618083003/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/618083003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:49745: + <int value="0" label="QUIC_DISK_CACHE_ENTRY_OPENED"/> nit: consider making the labels simply "OPENED" and "CLOSED"
Thanks much Ryan, raman https://codereview.chromium.org/618083003/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/618083003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:49745: + <int value="0" label="QUIC_DISK_CACHE_ENTRY_OPENED"/> On 2014/09/30 22:39:33, Ryan Hamilton wrote: > nit: consider making the labels simply "OPENED" and "CLOSED" Done.
rtenneti@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya Sherman, It looks like Alexei is away. Could you take a look at histograms.xml changes? thanks raman
LGTM % nits. https://codereview.chromium.org/618083003/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/618083003/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:16823: + <summary>Tracks the open and close state of the disk cache entry.</summary> nit: Please document when this is recorded. I *think* that's something like "Recorded each time the state changes". https://codereview.chromium.org/618083003/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:16823: + <summary>Tracks the open and close state of the disk cache entry.</summary> nit: What is "the disk cache entry"? Is there just a single entry? Perhaps you meant something like "each disk cache entry" or "disk cache entries"? https://codereview.chromium.org/618083003/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:49746: + <int value="1" label="CLOSED"/> nit: These are meant to be human-readability-oriented labels, so I'd change the casing to "Opened" and "Closed"
Hi Ilya, Updated the doc in histograms.xml. PTAL. thanks raman https://codereview.chromium.org/618083003/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/618083003/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:16823: + <summary>Tracks the open and close state of the disk cache entry.</summary> On 2014/10/01 19:47:36, Ilya Sherman wrote: > nit: What is "the disk cache entry"? Is there just a single entry? Perhaps you > meant something like "each disk cache entry" or "disk cache entries"? Done. https://codereview.chromium.org/618083003/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:16823: + <summary>Tracks the open and close state of the disk cache entry.</summary> On 2014/10/01 19:47:36, Ilya Sherman wrote: > nit: Please document when this is recorded. I *think* that's something like > "Recorded each time the state changes". We record each time disk cache entry is either opened or closed. Updated the document. PTAL. https://codereview.chromium.org/618083003/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:49746: + <int value="1" label="CLOSED"/> On 2014/10/01 19:47:36, Ilya Sherman wrote: > nit: These are meant to be human-readability-oriented labels, so I'd change the > casing to "Opened" and "Closed" Done.
LGTM % nits: https://codereview.chromium.org/618083003/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/618083003/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:16842: + Tracks the opening and closing of the disk cache entries. Recorded each time nit: "the disk cache entries" -> "disk cache entries" https://codereview.chromium.org/618083003/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:16843: + disk cache entry is either opened or closed. nit: "disk cache entry" -> "a disk cache entry"
The CQ bit was checked by rtenneti@chromium.org
The CQ bit was unchecked by rtenneti@chromium.org
https://codereview.chromium.org/618083003/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/618083003/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:16842: + Tracks the opening and closing of the disk cache entries. Recorded each time On 2014/10/01 22:02:08, Ilya Sherman wrote: > nit: "the disk cache entries" -> "disk cache entries" Done. https://codereview.chromium.org/618083003/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:16843: + disk cache entry is either opened or closed. On 2014/10/01 22:02:08, Ilya Sherman wrote: > nit: "disk cache entry" -> "a disk cache entry" Done.
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/618083003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/63283) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/618083003/140001
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as 4e25bc71cf274114911bfd365f4cad05cb5ceec8
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ddd25bc416a00c5cfd4a2b5051e514671088f639 Cr-Commit-Position: refs/heads/master@{#297776} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
