[Offline pages] Resetting offline page metadata store if initial load fails

This issue is obsolete.
https://codereview.chromium.org/2497703002 solved the reset problem.


[Offline pages] Resetting offline page metadata store if initial load fails

* Updates the reset functionality of OPMStoreSQL
* Updates tests
* Makes OPM attempt a reset if the initial read fails
* Adds relevant tests for reset.

BUG=645522

[fgorski, 2016-10-04 22:34:27]

The CQ bit was checked by fgorski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 22:35:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 23:35:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 23:35:26]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-10-05 17:09:57]

fgorski@chromium.org changed reviewers:
+ dimich@chromium.org

[fgorski, 2016-10-05 17:09:58]

PTAL.

[fgorski, 2016-10-05 17:10:49]

Description was changed from

==========
[Offline pages] Resetting offline page metadata store if initial load fails

BUG=645522
==========

to

==========
[Offline pages] Resetting offline page metadata store if initial load fails

* Updates the reset functionality of OPMStoreSQL
* Updates tests
* Makes OPM attempt a reset if the initial read fails
* Adds relevant tests for reset.

BUG=645522
==========

[fgorski, 2016-10-06 22:54:10]

fgorski@chromium.org changed reviewers:
+ dewittj@chromium.org

[fgorski, 2016-10-06 22:54:10]

Adding Justin who shown interest in the morning.

[fgorski, 2016-10-06 22:58:06]

fgorski@chromium.org changed reviewers:
+ jochen@chromium.org

[fgorski, 2016-10-06 22:58:07]

Adding jochen@ for histograms

[jochen (gone - plz use gerrit), 2016-10-10 12:47:33]

I can only review UseCounter additions to histograms.xml, sorry, please add a
metrics OWNER instead

[Dmitry Titov, 2016-10-10 16:38:59]

I'd like to chat (offline) about overall approach. I'd like to clarify some
corner cases...

[fgorski, 2016-10-10 21:03:38]

fgorski@chromium.org changed reviewers:
+ mpearson@chromium.org
- jochen@chromium.org

[fgorski, 2016-10-10 21:03:38]

Replacing jochen@ with Mark. (Tools failed me.) Please review histograms.

[Mark P, 2016-10-11 23:08:10]

I am away today and yesterday; I will review this tomorrow.

-m

[Mark P, 2016-10-12 16:04:13]

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:831:
UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount",
I notice this moved from offline_page_metadata_store_sql.cc.  Is it still
emitted under exactly the same conditions  as before? (including error states)?

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_store_types.h (right):

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
components/offline_pages/offline_store_types.h:24: NOT_LOADED,        // Store
is not loaded yet.
minor nit: put = 0, =1, etc. to help further emphasize that the actual values
are important.

[Mark P, 2016-10-12 16:06:31]

https://codereview.chromium.org/2384423003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2384423003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:38808: +    store.
It feels a bit odd to me to mix these two interpretations.  I'd think two
separate histograms would be cleaner and cleaer.  Why did you do it this way?

[fgorski, 2016-10-12 20:43:17]

The CQ bit was checked by fgorski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 20:43:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fgorski, 2016-10-12 20:44:09]

OK. I was able to address one directly, we can discuss 2 vs 1 UMAs.

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:831:
UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount",
On 2016/10/12 16:04:12, Mark P (sick) wrote:
> I notice this moved from offline_page_metadata_store_sql.cc.  Is it still
> emitted under exactly the same conditions  as before? (including error
states)?

Yes, because we are going to be calling the other method more often then simply
initialization in future. This had to be moved to post initialization call. We
would be reporting it far too much otherwise. Thanks for checking on that.

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_store_types.h (right):

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
components/offline_pages/offline_store_types.h:24: NOT_LOADED,        // Store
is not loaded yet.
On 2016/10/12 16:04:13, Mark P (sick) wrote:
> minor nit: put = 0, =1, etc. to help further emphasize that the actual values
> are important.

done. Is that a new requirement? I don't remember being asked that before.

https://codereview.chromium.org/2384423003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2384423003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:38808: +    store.
On 2016/10/12 16:06:31, Mark P (sick) wrote:
> It feels a bit odd to me to mix these two interpretations.  I'd think two
> separate histograms would be cleaner and cleaer.  Why did you do it this way?

This histogram covers the state of the store. If the store reset fails at some
point I will not be retrying anymore and some invalid state persists. It is not
covering 2 separate operations, but the state of store.

Do you feel strongly I report on initialization and reset using independent
status UMAs?

[commit-bot: I haz the power, 2016-10-12 22:06:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 22:06:28]

Dry run: This issue passed the CQ dry run.

[Mark P, 2016-10-12 22:12:15]

histograms.xml lgtm with one ongoing concern

--mark

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:831:
UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount",
On 2016/10/12 20:44:09, fgorski wrote:
> On 2016/10/12 16:04:12, Mark P (sick) wrote:
> > I notice this moved from offline_page_metadata_store_sql.cc.  Is it still
> > emitted under exactly the same conditions  as before? (including error
> states)?
> 
> Yes, because we are going to be calling the other method more often then
simply
> initialization in future. This had to be moved to post initialization call. We
> would be reporting it far too much otherwise. Thanks for checking on that.

Acknowledged.

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_store_types.h (right):

https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/of...
components/offline_pages/offline_store_types.h:24: NOT_LOADED,        // Store
is not loaded yet.
On 2016/10/12 20:44:09, fgorski wrote:
> On 2016/10/12 16:04:13, Mark P (sick) wrote:
> > minor nit: put = 0, =1, etc. to help further emphasize that the actual
values
> > are important.
> 
> done. Is that a new requirement? I don't remember being asked that before.

It's not a requirement, but it is a good practice.  We haven't been consistent
about asking people to do this, which is why you may have not seen anyone
mention it before.

https://codereview.chromium.org/2384423003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2384423003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:38808: +    store.
On 2016/10/12 20:44:09, fgorski wrote:
> On 2016/10/12 16:06:31, Mark P (sick) wrote:
> > It feels a bit odd to me to mix these two interpretations.  I'd think two
> > separate histograms would be cleaner and cleaer.  Why did you do it this
way?
> 
> This histogram covers the state of the store. If the store reset fails at some
> point I will not be retrying anymore and some invalid state persists. It is
not
> covering 2 separate operations, but the state of store.
> 
> Do you feel strongly I report on initialization and reset using independent
> status UMAs?

I don't feel strongly.  I think I can imagine cases when you might want to just
the, say, initial load case and not be able to get it, but you'd know better
than me.

If you want to keep on histogram, please briefly explain here what causes a
reset of the offline page metadata store.  (Is this an explicit action by the
user?  Happens periodically?  etc.)

[Dmitry Titov, 2016-11-15 20:23:46]

https://codereview.chromium.org/2384423003/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2384423003/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:848:
"OfflinePages.StoreState", static_cast<int32_t>(store_->state()),
There is also another point of logging same UMA in OnGetOfflinePagesDoneForInit,
which will double-count successes I think. Probably should only log here if
(!reset_success)? Or do you want a separate UMA for successes of the reset?

[fgorski, 2016-11-18 19:04:17]

Description was changed from

==========
[Offline pages] Resetting offline page metadata store if initial load fails

* Updates the reset functionality of OPMStoreSQL
* Updates tests
* Makes OPM attempt a reset if the initial read fails
* Adds relevant tests for reset.

BUG=645522
==========

to

==========
This issue is obsolete.
https://codereview.chromium.org/2497703002 solved the reset problem.


[Offline pages] Resetting offline page metadata store if initial load fails

* Updates the reset functionality of OPMStoreSQL
* Updates tests
* Makes OPM attempt a reset if the initial read fails
* Adds relevant tests for reset.

BUG=645522
==========