Remove the provisional store and just store the staged log when writing to

Remove the provisional store and just keep the staged log in the queue while uploading it.

BUG=372911
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285684

[Steven Holte, 2014-05-16 19:56:21]

Added some additional changes I had thought I would leave to a separate patch,
but actually require them here:

In the original code, if we have an initial stability log, we
LoadPersistedUnsentLogs, create the stability log, persist, send it, persist,
create the initial metrics logs and send it.

If we don't, we create the initial metrics log, stage it, then load old logs,
then send the initial log.  This is a problem if we store the staged log in the
list because we can't load logs after it's staged, but we can just load logs
before staging it.

For simplicity, I've extracted the LoadPersistedUnsentLogs so it happens in
InitializeMetricsState regardless of whether we have a stability log or not.

I've also included a fix for crbug/328417, by making it PrepareInitialMetricsLog
call PersistUnsentLogs and calling it again when that log is sent.  It should
also be cheap to call it after sending a log during SENDING_CURRENT_LOGS too,
because we will just write an empty list.

[Alexei Svitkine (slow), 2014-05-16 20:12:27]

Steve, do you mind holding off on this change until after next week?

There will be a lot of changes happening to the metrics service code next
week for the compentization hackathon and I'd like us to not mix functional
changes so close in time to refactoring changes (to make it easier to
diagnose regressions if they happen).

For this particular change, I don't think it should be affected very much
by the refactoring (the parts being touched here aren't those that we'll be
refactoring during the hackathon), so I think after next week the rebase
for this wouldn't have conflicts - except possibly the metrics_service.cc
part would be in components.


On Fri, May 16, 2014 at 3:56 PM, <holte@chromium.org> wrote:

> Added some additional changes I had thought I would leave to a separate
> patch,
> but actually require them here:
>
> In the original code, if we have an initial stability log, we
> LoadPersistedUnsentLogs, create the stability log, persist, send it,
> persist,
> create the initial metrics logs and send it.
>
> If we don't, we create the initial metrics log, stage it, then load old
> logs,
> then send the initial log.  This is a problem if we store the staged log
> in the
> list because we can't load logs after it's staged, but we can just load
> logs
> before staging it.
>
> For simplicity, I've extracted the LoadPersistedUnsentLogs so it happens in
> InitializeMetricsState regardless of whether we have a stability log or
> not.
>
> I've also included a fix for crbug/328417, by making it
> PrepareInitialMetricsLog
> call PersistUnsentLogs and calling it again when that log is sent.  It
> should
> also be cheap to call it after sending a log during SENDING_CURRENT_LOGS
> too,
> because we will just write an empty list.
>
> https://codereview.chromium.org/290103005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Steven Holte, 2014-05-16 20:31:34]

On 2014/05/16 20:12:27, Alexei Svitkine wrote:
> Steve, do you mind holding off on this change until after next week?
> 
> There will be a lot of changes happening to the metrics service code next
> week for the compentization hackathon and I'd like us to not mix functional
> changes so close in time to refactoring changes (to make it easier to
> diagnose regressions if they happen).
> 
> For this particular change, I don't think it should be affected very much
> by the refactoring (the parts being touched here aren't those that we'll be
> refactoring during the hackathon), so I think after next week the rebase
> for this wouldn't have conflicts - except possibly the metrics_service.cc
> part would be in components.
> 
> 
> On Fri, May 16, 2014 at 3:56 PM, <mailto:holte@chromium.org> wrote:
> 
> > Added some additional changes I had thought I would leave to a separate
> > patch,
> > but actually require them here:
> >
> > In the original code, if we have an initial stability log, we
> > LoadPersistedUnsentLogs, create the stability log, persist, send it,
> > persist,
> > create the initial metrics logs and send it.
> >
> > If we don't, we create the initial metrics log, stage it, then load old
> > logs,
> > then send the initial log.  This is a problem if we store the staged log
> > in the
> > list because we can't load logs after it's staged, but we can just load
> > logs
> > before staging it.
> >
> > For simplicity, I've extracted the LoadPersistedUnsentLogs so it happens in
> > InitializeMetricsState regardless of whether we have a stability log or
> > not.
> >
> > I've also included a fix for crbug/328417, by making it
> > PrepareInitialMetricsLog
> > call PersistUnsentLogs and calling it again when that log is sent.  It
> > should
> > also be cheap to call it after sending a log during SENDING_CURRENT_LOGS
> > too,
> > because we will just write an empty list.
> >
> > https://codereview.chromium.org/290103005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ok, I will hold this change off until after next week.

[Alexei Svitkine (slow), 2014-06-26 17:16:48]

Can you re-base the CL now that the componentization changes are in?

[Steven Holte, 2014-06-27 02:11:54]

On 2014/06/26 17:16:48, Alexei Svitkine wrote:
> Can you re-base the CL now that the componentization changes are in?

Rebased, but have not retested.

[Alexei Svitkine (slow), 2014-07-03 21:24:23]

Can you split this into several CLs so that it's easier to review/diagnose
issues if it causes them?

For example, one of the CLs should just change the
PersistUnsentLogs()/LoadPersistedLogs() flow in metrics_service.cc without other
changes.

[Steven Holte, 2014-07-14 17:44:28]

On 2014/07/03 21:24:23, Alexei Svitkine wrote:
> Can you split this into several CLs so that it's easier to review/diagnose
> issues if it causes them?
> 
> For example, one of the CLs should just change the
> PersistUnsentLogs()/LoadPersistedLogs() flow in metrics_service.cc without
other
> changes.

Split to change to LoadPersistedLogs() flow
https://codereview.chromium.org/384063004

[Steven Holte, 2014-07-24 22:05:25]

Rebased ontop of
https://codereview.chromium.org/384063004/
https://codereview.chromium.org/397443004/

[Alexei Svitkine (slow), 2014-07-25 15:16:11]

lgtm, nice!

[Steven Holte, 2014-07-25 19:27:10]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2014-07-25 19:28:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/holte@chromium.org/290103005/120001

[commit-bot: I haz the power, 2014-07-25 21:30:31]

Change committed as 285684