Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2497703002:
[Offline pages] Resetting offline page metadata store to handle LOAD/INIT failures (Closed)
|
Created:
4 years, 1 month ago by fgorski Modified:
4 years, 1 month 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/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Resetting offline page metadata store to handle LOAD/INIT failures
* Adds a store reset to offline page model, in case the initial load
fails with STORE_LOAD_FAILED status
* Adds Initialize method to the store to handle STORE_INIT_FAILED.
* Postpones OfflinePageModel actions until after initialization of
the store succeeds/fails.
* Adds tests to cover successful reset and unsuccessful one.
* Updates tests that needed adjusting.
BUG=605249
Committed: https://crrev.com/9c4ec432de012e9f6c99dfc55cd0535694a4e828
Cr-Commit-Position: refs/heads/master@{#432984}
Patch Set 1 #Patch Set 2 : Updating StoreResetSuccessful test and OPTestStore #Patch Set 3 : Addressing STORE_INIT_FAILED #
Total comments: 6
Patch Set 4 : Addressing comments form self-review #
Total comments: 6
Patch Set 5 : Addressing feedback from jianli@ and dimich@ #
Total comments: 14
Patch Set 6 : Addressing CR feedback from Jian #Patch Set 7 : Rebasing #Patch Set 8 : Fixing newly added test #Messages
Total messages: 53 (35 generated)
The CQ bit was checked by fgorski@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 checked by fgorski@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...
fgorski@chromium.org changed reviewers: + jianli@chromium.org
PTAL This is part 1 (outside of our Init discussion)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [Offline pages] Resetting offline page metadata store upon STORE_LOAD_FAILED * Adds a store reset to offline page model, in case the initial load fails with STORE_LOAD_FAILED status * Adds tests to cover successful reset and unsuccessful one. BUG=605249 ========== to ========== [Offline pages] Resetting offline page metadata store upon STORE_LOAD_FAILED * Adds a store reset to offline page model, in case the initial load fails with STORE_LOAD_FAILED status * Adds Initialize method to the store to handle STORE_INIT_FAILED. * Postpones OfflinePageModel actions until after initialization of the store succeeds/fails. * Adds tests to cover successful reset and unsuccessful one. * Updates tests that needed adjusting. BUG=605249 ==========
Description was changed from ========== [Offline pages] Resetting offline page metadata store upon STORE_LOAD_FAILED * Adds a store reset to offline page model, in case the initial load fails with STORE_LOAD_FAILED status * Adds Initialize method to the store to handle STORE_INIT_FAILED. * Postpones OfflinePageModel actions until after initialization of the store succeeds/fails. * Adds tests to cover successful reset and unsuccessful one. * Updates tests that needed adjusting. BUG=605249 ========== to ========== [Offline pages] Resetting offline page metadata store to handle LOAD/INIT failures * Adds a store reset to offline page model, in case the initial load fails with STORE_LOAD_FAILED status * Adds Initialize method to the store to handle STORE_INIT_FAILED. * Postpones OfflinePageModel actions until after initialization of the store succeeds/fails. * Adds tests to cover successful reset and unsuccessful one. * Updates tests that needed adjusting. BUG=605249 ==========
self review https://codereview.chromium.org/2497703002/diff/40001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.h (right): https://codereview.chromium.org/2497703002/diff/40001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.h:81: void OnResetDone(const InitializeCallback& callback, StoreState state); needs to be removed. https://codereview.chromium.org/2497703002/diff/40001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2497703002/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:855: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( Move to CompleteLoad, as we want to clear storage even when the store load was unsuccessful. https://codereview.chromium.org/2497703002/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:1069: void OfflinePageModelImpl::ClearStorageIfNeeded( make this method call itself.
The CQ bit was checked by fgorski@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...
Jian, This patch is now ready for your review. It covers both cases: INIT/LOAD_FAILED, and implements Initialization per your comments in design document for M56. PTAL. https://codereview.chromium.org/2497703002/diff/40001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.h (right): https://codereview.chromium.org/2497703002/diff/40001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.h:81: void OnResetDone(const InitializeCallback& callback, StoreState state); On 2016/11/14 21:59:47, fgorski wrote: > needs to be removed. Done. https://codereview.chromium.org/2497703002/diff/40001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2497703002/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:855: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( On 2016/11/14 21:59:47, fgorski wrote: > Move to CompleteLoad, as we want to clear storage even when the store load was > unsuccessful. Done. https://codereview.chromium.org/2497703002/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:1069: void OfflinePageModelImpl::ClearStorageIfNeeded( On 2016/11/14 21:59:47, fgorski wrote: > make this method call itself. Done through Post...Task method instead. Intention was to either run now or post delayed, but we already had a necessary method for that.
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...)
fgorski@chromium.org changed reviewers: + dimich@google.com
I am looking at a failing test, but please review this change. Obsoletes earlier store reset patch: https://codereview.chromium.org/2384423003/
https://codereview.chromium.org/2497703002/diff/60001/components/offline_page... File components/offline_pages/offline_page_metadata_store.h (right): https://codereview.chromium.org/2497703002/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:43: typedef base::Callback<void(StoreState)> InitializeCallback; nit: consider switching to use "using". Also, it would be better to use bool instead of StoreState since 2 out of 4 StoreState enums do not apply. https://codereview.chromium.org/2497703002/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:71: virtual void Reset(const InitializeCallback& callback) = 0; Better to introduce ResetCallback to avoid the confusion. https://codereview.chromium.org/2497703002/diff/60001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2497703002/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:445: state = InitDatabase(db, db_file_path) ? StoreState::LOADED I think we should first get back to main thread upon destroying database. From there, we should recreate sql::Connection instance, instead of reusing an existing one, and then do the initialization. I have concern that some fields in sql::Connection instance are not reset. For example, commit_time_histogram_ and etc which might lead to incorrect data.
The CQ bit was checked by fgorski@chromium.org to run a CQ dry run
The CQ bit was checked by fgorski@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...
Updated. PTAL. https://codereview.chromium.org/2497703002/diff/60001/components/offline_page... File components/offline_pages/offline_page_metadata_store.h (right): https://codereview.chromium.org/2497703002/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:43: typedef base::Callback<void(StoreState)> InitializeCallback; On 2016/11/15 21:48:38, jianli wrote: > nit: consider switching to use "using". > > Also, it would be better to use bool instead of StoreState since 2 out of 4 > StoreState enums do not apply. I switched to bool based callbacks (after split). I will not be switching from typedef to using in this patch, because of the style of the file, but we can have a separate effort for that. https://codereview.chromium.org/2497703002/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:71: virtual void Reset(const InitializeCallback& callback) = 0; On 2016/11/15 21:48:38, jianli wrote: > Better to introduce ResetCallback to avoid the confusion. Done. https://codereview.chromium.org/2497703002/diff/60001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2497703002/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:445: state = InitDatabase(db, db_file_path) ? StoreState::LOADED On 2016/11/15 21:48:38, jianli wrote: > I think we should first get back to main thread upon destroying database. From > there, we should recreate sql::Connection instance, instead of reusing an > existing one, and then do the initialization. I have concern that some fields in > sql::Connection instance are not reset. For example, commit_time_histogram_ and > etc which might lead to incorrect data. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:572: db_.reset(nullptr); nit: do we really need nullptr? https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.h (right): https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.h:69: void Reset(const InitializeCallback& callback) override; Will this be ResetCallback? https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:818: int kResetAttemptsLeft = 1; nit: add const https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:878: void OfflinePageModelImpl::CompleteLoad() { It is a bit hard to understand the difference from the names OnLoadDone and CompleteLoad. Since CompleteLoad applies to both success and failure case, the name CompleteLoad seems to only suggest the successful load (also given that no success/result parameter is passed). How about renaming it to something like FinalizeAfterLoadAttempt? https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:888: // Run all the delayed tasks. Also mention that the delay task will check for the load state and return earlier. https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:893: // Clear storage. Is it still needed when failed to load? https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:1114: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( Is is possible we can post multiple clear tasks, esp when we now call PostClearStorageIfNeeded more frequently?
The CQ bit was checked by fgorski@chromium.org to run a CQ dry run
Patchset #6 (id:100001) has been deleted
Addressed. PTAL https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:572: db_.reset(nullptr); On 2016/11/16 01:34:50, jianli wrote: > nit: do we really need nullptr? Looks like we don't. I thought that this was C++17 feature and we don't support it yet, but you live you learn. I'll be watching the bots. https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.h (right): https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.h:69: void Reset(const InitializeCallback& callback) override; On 2016/11/16 01:34:50, jianli wrote: > Will this be ResetCallback? Done. https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:818: int kResetAttemptsLeft = 1; On 2016/11/16 01:34:50, jianli wrote: > nit: add const Done. https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:878: void OfflinePageModelImpl::CompleteLoad() { On 2016/11/16 01:34:50, jianli wrote: > It is a bit hard to understand the difference from the names OnLoadDone and > CompleteLoad. > > Since CompleteLoad applies to both success and failure case, the name > CompleteLoad seems to only suggest the successful load (also given that no > success/result parameter is passed). How about renaming it to something like > FinalizeAfterLoadAttempt? Done. Renamed: OnLoadDone -> OnInitialGetOfflinePagesDone CompleteLoad -> FinalizeModelLoad https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:888: // Run all the delayed tasks. On 2016/11/16 01:34:50, jianli wrote: > Also mention that the delay task will check for the load state and return > earlier. I am not sure what you are referring to. Nothing about this code changed so I'd preferably leave it the way it was. https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:893: // Clear storage. On 2016/11/16 01:34:50, jianli wrote: > Is it still needed when failed to load? Yes, if I fail to reset the store I would like to reclaim storage. Since we are already in situation where old DB cannot be recovered, that would happen anyway with the successful reset. This is actually calling that code earlier. (Perhaps freeing some space for the user). https://codereview.chromium.org/2497703002/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:1114: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( On 2016/11/16 01:34:50, jianli wrote: > Is is possible we can post multiple clear tasks, esp when we now call > PostClearStorageIfNeeded more frequently? We don't call anything more frequently. This is covering line 853 from the old code (scheduling with a delay). And the rest is left intact.
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.
lgtm
The CQ bit was checked by fgorski@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
Failed to apply patch for components/offline_pages/offline_page_model_impl.h:
While running git apply --index -p1;
error: patch failed: components/offline_pages/offline_page_model_impl.h:150
error: components/offline_pages/offline_page_model_impl.h: patch does not
apply
Patch: components/offline_pages/offline_page_model_impl.h
Index: components/offline_pages/offline_page_model_impl.h
diff --git a/components/offline_pages/offline_page_model_impl.h
b/components/offline_pages/offline_page_model_impl.h
index
bb5c972ca66d5eaa75c96dd20af78925cd14d9e0..4bc1853ce5c97f06e027f19197a8008e504a361f
100644
--- a/components/offline_pages/offline_page_model_impl.h
+++ b/components/offline_pages/offline_page_model_impl.h
@@ -150,9 +150,16 @@ class OfflinePageModelImpl : public OfflinePageModel,
public KeyedService {
void CheckMetadataConsistency();
// Callback for loading pages from the offline page metadata store.
- void OnLoadDone(const base::TimeTicks& start_time,
- OfflinePageMetadataStore::LoadStatus load_status,
- const std::vector<OfflinePageItem>& offline_pages);
+ void OnStoreInitialized(const base::TimeTicks& start_time,
+ int reset_attempts_left,
+ bool success);
+ void OnStoreResetDone(const base::TimeTicks& start_time,
+ int reset_attempts_left,
+ bool success);
+ void OnInitialGetOfflinePagesDone(
+ const base::TimeTicks& start_time,
+ const std::vector<OfflinePageItem>& offline_pages);
+ void FinalizeModelLoad();
// Steps for saving a page offline.
void OnCreateArchiveDone(const GURL& requested_url,
@@ -242,7 +249,7 @@ class OfflinePageModelImpl : public OfflinePageModel, public
KeyedService {
OfflinePageStorageManager::ClearStorageResult result);
// Post task to clear storage.
- void PostClearStorageIfNeededTask();
+ void PostClearStorageIfNeededTask(bool delayed);
// Check if |offline_page| should be removed on cache reset by user.
bool IsRemovedOnCacheReset(const OfflinePageItem& offline_page) const;
The CQ bit was checked by fgorski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org Link to the patchset: https://codereview.chromium.org/2497703002/#ps140001 (title: "Rebasing")
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by fgorski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org Link to the patchset: https://codereview.chromium.org/2497703002/#ps160001 (title: "Fixing newly added 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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by fgorski@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.
Description was changed from ========== [Offline pages] Resetting offline page metadata store to handle LOAD/INIT failures * Adds a store reset to offline page model, in case the initial load fails with STORE_LOAD_FAILED status * Adds Initialize method to the store to handle STORE_INIT_FAILED. * Postpones OfflinePageModel actions until after initialization of the store succeeds/fails. * Adds tests to cover successful reset and unsuccessful one. * Updates tests that needed adjusting. BUG=605249 ========== to ========== [Offline pages] Resetting offline page metadata store to handle LOAD/INIT failures * Adds a store reset to offline page model, in case the initial load fails with STORE_LOAD_FAILED status * Adds Initialize method to the store to handle STORE_INIT_FAILED. * Postpones OfflinePageModel actions until after initialization of the store succeeds/fails. * Adds tests to cover successful reset and unsuccessful one. * Updates tests that needed adjusting. BUG=605249 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Resetting offline page metadata store to handle LOAD/INIT failures * Adds a store reset to offline page model, in case the initial load fails with STORE_LOAD_FAILED status * Adds Initialize method to the store to handle STORE_INIT_FAILED. * Postpones OfflinePageModel actions until after initialization of the store succeeds/fails. * Adds tests to cover successful reset and unsuccessful one. * Updates tests that needed adjusting. BUG=605249 ========== to ========== [Offline pages] Resetting offline page metadata store to handle LOAD/INIT failures * Adds a store reset to offline page model, in case the initial load fails with STORE_LOAD_FAILED status * Adds Initialize method to the store to handle STORE_INIT_FAILED. * Postpones OfflinePageModel actions until after initialization of the store succeeds/fails. * Adds tests to cover successful reset and unsuccessful one. * Updates tests that needed adjusting. BUG=605249 Committed: https://crrev.com/9c4ec432de012e9f6c99dfc55cd0535694a4e828 Cr-Commit-Position: refs/heads/master@{#432984} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9c4ec432de012e9f6c99dfc55cd0535694a4e828 Cr-Commit-Position: refs/heads/master@{#432984} |
