Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1982433002:
Make SimpleCache the default backend on ChromeOS/Linux. (Closed)
DescriptionMake SimpleCache the default backend on ChromeOS/Linux.
The current beta on these platforms is looking good, so we will
launch. However, validating simple cache in stable will take some
time. Thus, I'm switching the default to launch, but leaving us with
the finch trial so we can claw back users if we run into unexpected
trouble.
In the m53 timeframe, we'll just switch to the new default.
R=asvitkine@chromium.org,mmenke@chromium.org
BUG=611648, 611647
Committed: https://crrev.com/5f957e5455e30682f8ee5df414cac08ec87fcddc
Cr-Commit-Position: refs/heads/master@{#393953}
Patch Set 1 #
Total comments: 2
Patch Set 2 : remediate #Patch Set 3 : better #
Total comments: 1
Patch Set 4 : listen to mmenke #Patch Set 5 : and fix it #Messages
Total messages: 18 (7 generated)
Description was changed from ========== Make SimpleCache the default backend on ChromeOS/Linux. The current beta on these platforms is looking good, so we will launch. However, validating simple cache in stable will take some time. Thus, I'm switching the default to launch, but leaving us with the finch trial so we can claw back users if we run into unexpected trouble. In the m53 timeframe, we'll just switch to the new default. R=asvitkine@chromium.org BUG=611648,611647 ========== to ========== Make SimpleCache the default backend on ChromeOS/Linux. The current beta on these platforms is looking good, so we will launch. However, validating simple cache in stable will take some time. Thus, I'm switching the default to launch, but leaving us with the finch trial so we can claw back users if we run into unexpected trouble. In the m53 timeframe, we'll just switch to the new default. R=asvitkine@chromium.org BUG=611648,611647 ==========
gavinp@chromium.org changed reviewers: + mmenke@chromium.org
Description was changed from ========== Make SimpleCache the default backend on ChromeOS/Linux. The current beta on these platforms is looking good, so we will launch. However, validating simple cache in stable will take some time. Thus, I'm switching the default to launch, but leaving us with the finch trial so we can claw back users if we run into unexpected trouble. In the m53 timeframe, we'll just switch to the new default. R=asvitkine@chromium.org BUG=611648,611647 ========== to ========== Make SimpleCache the default backend on ChromeOS/Linux. The current beta on these platforms is looking good, so we will launch. However, validating simple cache in stable will take some time. Thus, I'm switching the default to launch, but leaving us with the finch trial so we can claw back users if we run into unexpected trouble. In the m53 timeframe, we'll just switch to the new default. R=asvitkine@chromium.org,mmenke@chromium.org BUG=611648,611647 ==========
mmenke: PTAL. I believe this is in keeping with our current launch plan; see crbug.com/490029 for details. Keeping the experiment lets us yank things back if we need to.
https://codereview.chromium.org/1982433002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1982433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_impl_io_data.cc:94: experiment_name == "ExperimentControl") { If enabled is the new behavior, then any experiment that changes this should have control group specify the new behavior. Also, suggests changing the check to StartsWith() per our best practices. You can also change the one below to be StartsWith() "ExperimentYes" to avoid needing the two =='s.
remediated per asvitkine's suggestions https://codereview.chromium.org/1982433002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1982433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_impl_io_data.cc:94: experiment_name == "ExperimentControl") { On 2016/05/13 18:40:51, Alexei Svitkine wrote: > If enabled is the new behavior, then any experiment that changes this should > have control group specify the new behavior. Good point. I've renamed the group "Disable" for the new world with Simple Cache everywhere. > > Also, suggests changing the check to StartsWith() per our best practices. You > can also change the one below to be StartsWith() "ExperimentYes" to avoid > needing the two =='s. Done.
lgtm
https://codereview.chromium.org/1982433002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1982433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:104: #endif // #if defined(OS_LINUX) || defined(OS_CHROMEOS) Can this just be: if (..."Disable"...) return net::CACHE_BACKEND_BLOCKFILE; if (..."ExperimentYes"...) return net::CACHE_BACKEND_SIMPLE; #ifdefs go here Making disable and enable experiment names per-platform doesn't seem to get us anything.
mmenke: how's this? Much narrower I think, with an obvious path to launch.
LGTM, much better! Have you sent out an email to net-dev about this (Or some public notice of the change?) Just something so our external consumers know about this change, and expect it if they're gathering their own histograms.
On 2016/05/16 19:35:46, mmenke wrote: > LGTM, much better! Have you sent out an email to net-dev about this (Or some > public notice of the change?) Just something so our external consumers know > about this change, and expect it if they're gathering their own histograms. No public notice yet. That's on the agenda for tomorrow. I'm excited about progressing along these steps. I'll also be switching the current stable to use simple cache in a 99% experiment shortly, too. I really appreciate your review; it really cleaned it up a lot. Nesting preprocessor directives is ugly and scary. Thank you for talking me off of that ledge.
The CQ bit was checked by gavinp@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/1982433002/#ps80001 (title: "and fix it")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982433002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982433002/80001
Message was sent while issue was closed.
Description was changed from ========== Make SimpleCache the default backend on ChromeOS/Linux. The current beta on these platforms is looking good, so we will launch. However, validating simple cache in stable will take some time. Thus, I'm switching the default to launch, but leaving us with the finch trial so we can claw back users if we run into unexpected trouble. In the m53 timeframe, we'll just switch to the new default. R=asvitkine@chromium.org,mmenke@chromium.org BUG=611648,611647 ========== to ========== Make SimpleCache the default backend on ChromeOS/Linux. The current beta on these platforms is looking good, so we will launch. However, validating simple cache in stable will take some time. Thus, I'm switching the default to launch, but leaving us with the finch trial so we can claw back users if we run into unexpected trouble. In the m53 timeframe, we'll just switch to the new default. R=asvitkine@chromium.org,mmenke@chromium.org BUG=611648,611647 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make SimpleCache the default backend on ChromeOS/Linux. The current beta on these platforms is looking good, so we will launch. However, validating simple cache in stable will take some time. Thus, I'm switching the default to launch, but leaving us with the finch trial so we can claw back users if we run into unexpected trouble. In the m53 timeframe, we'll just switch to the new default. R=asvitkine@chromium.org,mmenke@chromium.org BUG=611648,611647 ========== to ========== Make SimpleCache the default backend on ChromeOS/Linux. The current beta on these platforms is looking good, so we will launch. However, validating simple cache in stable will take some time. Thus, I'm switching the default to launch, but leaving us with the finch trial so we can claw back users if we run into unexpected trouble. In the m53 timeframe, we'll just switch to the new default. R=asvitkine@chromium.org,mmenke@chromium.org BUG=611648,611647 Committed: https://crrev.com/5f957e5455e30682f8ee5df414cac08ec87fcddc Cr-Commit-Position: refs/heads/master@{#393953} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5f957e5455e30682f8ee5df414cac08ec87fcddc Cr-Commit-Position: refs/heads/master@{#393953} |
