Fix a bug that causes the metrics loop to be broken.

Fix a bug that causes the metrics loop to be broken.

The logs upload loop can be broken if a log upload fails while in state
SENDING_INITIAL_METRICS_LOG.

BUG=441460
Committed: https://crrev.com/09a7a5528847ce83687584adee700d23744491ca
Cr-Commit-Position: refs/heads/master@{#308021}

[Steven Holte, 2014-12-11 20:23:36]

holte@chromium.org changed reviewers:
+ asvitkine@chromium.org, jwd@chromium.org

[jwd, 2014-12-11 20:39:54]

https://codereview.chromium.org/799613002/diff/1/components/metrics/metrics_s...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/799613002/diff/1/components/metrics/metrics_s...
components/metrics/metrics_service.cc:1121: // two as a single unit of work as
far as the scheduler is concerned).
Please update this comment.

[Steven Holte, 2014-12-11 20:46:54]

https://codereview.chromium.org/799613002/diff/1/components/metrics/metrics_s...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/799613002/diff/1/components/metrics/metrics_s...
components/metrics/metrics_service.cc:1121: // two as a single unit of work as
far as the scheduler is concerned).
On 2014/12/11 20:39:54, Jesse Doherty wrote:
> Please update this comment.

Slightly tweaked the comment, but it's basically consistent with the intended
behaviour.  The problem was state_ could be SENDING_INITIAL_METRICS_LOG for
other reasons (i.e. there was no initial stability log, but we the first upload
attempt failed due to timeout).

[Alexei Svitkine (slow), 2014-12-11 23:13:26]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-12-11 23:13:26]

lgtm

[commit-bot: I haz the power, 2014-12-11 23:14:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/799613002/20001

[Alexei Svitkine (slow), 2014-12-11 23:42:45]

LGTM,  thanks!

Not for this CL, but I think it would be good to think about how we can
test these flows better - e.g. by mocking the actual connections and
simulating states. Maybe after we clean this up and enable the trial.

On Thu, Dec 11, 2014 at 3:46 PM, <holte@chromium.org> wrote:

>
> https://codereview.chromium.org/799613002/diff/1/
> components/metrics/metrics_service.cc
> File components/metrics/metrics_service.cc (right):
>
> https://codereview.chromium.org/799613002/diff/1/
> components/metrics/metrics_service.cc#newcode1121
> components/metrics/metrics_service.cc:1121: // two as a single unit of
> work as far as the scheduler is concerned).
> On 2014/12/11 20:39:54, Jesse Doherty wrote:
>
>> Please update this comment.
>>
>
> Slightly tweaked the comment, but it's basically consistent with the
> intended behaviour.  The problem was state_ could be
> SENDING_INITIAL_METRICS_LOG for other reasons (i.e. there was no initial
> stability log, but we the first upload attempt failed due to timeout).
>
> https://codereview.chromium.org/799613002/
>

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

[chromium-reviews, 2014-12-11 23:44:47]

Hm, it looks like chromium-reviews ignored your email.  Can you add the
LGTM to the CL?

On Thu, Dec 11, 2014 at 2:18 PM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> LGTM,  thanks!
>
> Not for this CL, but I think it would be good to think about how we can
> test these flows better - e.g. by mocking the actual connections and
> simulating states. Maybe after we clean this up and enable the trial.
>
> On Thu, Dec 11, 2014 at 3:46 PM, <holte@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/799613002/diff/1/
>> components/metrics/metrics_service.cc
>> File components/metrics/metrics_service.cc (right):
>>
>> https://codereview.chromium.org/799613002/diff/1/
>> components/metrics/metrics_service.cc#newcode1121
>> components/metrics/metrics_service.cc:1121: // two as a single unit of
>> work as far as the scheduler is concerned).
>> On 2014/12/11 20:39:54, Jesse Doherty wrote:
>>
>>> Please update this comment.
>>>
>>
>> Slightly tweaked the comment, but it's basically consistent with the
>> intended behaviour.  The problem was state_ could be
>> SENDING_INITIAL_METRICS_LOG for other reasons (i.e. there was no initial
>> stability log, but we the first upload attempt failed due to timeout).
>>
>> https://codereview.chromium.org/799613002/
>>
>
>

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

[commit-bot: I haz the power, 2014-12-12 01:33:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-12 01:34:17]

Patchset 2 (id:??) landed as
https://crrev.com/09a7a5528847ce83687584adee700d23744491ca
Cr-Commit-Position: refs/heads/master@{#308021}