GCM Directive fix

GCM Directive fix. We need to handle directives without op ids, but we should limit their TTL.

BUG=431871
Committed: https://crrev.com/9baadc9a0fe2f708d553070605d388547f5c3446
Cr-Commit-Position: refs/heads/master@{#303542}

[Charlie, 2014-11-10 21:32:01]

ckehoe@chromium.org changed reviewers:
+ rkc@chromium.org

[Charlie, 2014-11-10 21:36:45]

On 2014/11/10 21:32:02, Charlie wrote:

Hey Rahul,

Ben and I tested GCM directives this morning, and they're basically working. We
just need to make this small fix. There's some server complexity involved in
looking up the relevant publish and subscribe IDs, and we'd have to issue a
separate directive for each one, which would be unnecessary overhead. So this
seems like the best compromise.

I also think the server should probably only send down 5-minute directives for
this. We'll only need more than 1 min in extreme cases, and at that point the
impact of Constellation is pretty lost. But this limit will be baked into Chrome
until March, so I'm not sure setting it lower than 10 min is a good idea.

- Charlie

[brazon, 2014-11-10 21:52:07]

On 2014/11/10 21:36:45, Charlie wrote:
> On 2014/11/10 21:32:02, Charlie wrote:
> 
> Hey Rahul,
> 
> Ben and I tested GCM directives this morning, and they're basically working.
We
> just need to make this small fix. There's some server complexity involved in
> looking up the relevant publish and subscribe IDs, and we'd have to issue a
> separate directive for each one, which would be unnecessary overhead. So this
> seems like the best compromise.
> 
> I also think the server should probably only send down 5-minute directives for
> this. We'll only need more than 1 min in extreme cases, and at that point the
> impact of Constellation is pretty lost. But this limit will be baked into
Chrome
> until March, so I'm not sure setting it lower than 10 min is a good idea.
> 
> - Charlie

I think 10 minutes is a good value since it matches how Android handles this
case.

[rkc, 2014-11-10 22:03:06]

I am not sure if this is a good idea. This means that *no one* can cancel these
directives. This doesn't sound like something we'd like to do. Even with GCM
directives, once we're done, we want to be able to clean up after ourselves
(even if via another GCM remove directive).

[rkc, 2014-11-10 22:04:09]

If we are going to have uncancelable directives, they should be very short,
maybe 15-30s or so. The server can always send multiple directives.

[Charlie, 2014-11-10 22:12:34]

I did have some similar concerns. How about using some kind of fingerprint
of the directive as the op ID? Like the SHA1 hash maybe?

I'm not opposed to a lower TTL limit. But due to network latency, it could
be dicey to go below 1 minute.


On Mon Nov 10 2014 at 2:04:10 PM <rkc@chromium.org> wrote:

> If we are going to have uncancelable directives, they should be very short,
> maybe 15-30s or so. The server can always send multiple directives.
>
>
> https://codereview.chromium.org/710183003/
>

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

[rkc, 2014-11-10 22:24:44]

I am okay with a 1m TTL as the cap. The server can of course be more
conservative if needed.

If you need this merged in, you should make this CL an absolute minimum of the
code change that is needed to go into M-40. i.e., no renames, refactorings, test
changes (unless they are needed for the code to compile), etc. You can do all of
those in a follow-up CL.

[chromium-reviews, 2014-11-10 22:29:38]

Ok I'll take out the header file changes. But since we're adding TTL
limiting functionality, I think we should add a test to ensure that it
works as intended.

Ben, are you OK with a 1-min TTL cap? I think it's more than enough for
Constellation. Will we be using GCM for anything else in M40?

On Mon Nov 10 2014 at 2:24:44 PM <rkc@chromium.org> wrote:

> I am okay with a 1m TTL as the cap. The server can of course be more
> conservative if needed.
>
> If you need this merged in, you should make this CL an absolute minimum of
> the
> code change that is needed to go into M-40. i.e., no renames, refactorings,
> test
> changes (unless they are needed for the code to compile), etc. You can do
> all of
> those in a follow-up CL.
>
>
> https://codereview.chromium.org/710183003/
>

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

[chromium-reviews, 2014-11-10 22:44:14]

FYI the server currently seems to be setting TTL = 30 seconds in GCM pings.

On Mon Nov 10 2014 at 2:29:37 PM Charlie Kehoe <ckehoe@google.com> wrote:

> Ok I'll take out the header file changes. But since we're adding TTL
> limiting functionality, I think we should add a test to ensure that it
> works as intended.
>
> Ben, are you OK with a 1-min TTL cap? I think it's more than enough for
> Constellation. Will we be using GCM for anything else in M40?
>
> On Mon Nov 10 2014 at 2:24:44 PM <rkc@chromium.org> wrote:
>
>> I am okay with a 1m TTL as the cap. The server can of course be more
>> conservative if needed.
>>
>> If you need this merged in, you should make this CL an absolute minimum of
>> the
>> code change that is needed to go into M-40. i.e., no renames,
>> refactorings,
>> test
>> changes (unless they are needed for the code to compile), etc. You can do
>> all of
>> those in a follow-up CL.
>>
>>
>> https://codereview.chromium.org/710183003/
>>
>

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

[rkc, 2014-11-10 22:52:54]

lgtm % comment

https://codereview.chromium.org/710183003/diff/80001/components/copresence/ha...
File components/copresence/handlers/directive_handler.cc (right):

https://codereview.chromium.org/710183003/diff/80001/components/copresence/ha...
components/copresence/handlers/directive_handler.cc:61: DVLOG(2) << "No
operation associated with directive. Setting op id to "
Since this is pretty much expected, make this DVLOG(3).

[Charlie, 2014-11-10 23:04:13]

https://codereview.chromium.org/710183003/diff/80001/components/copresence/ha...
File components/copresence/handlers/directive_handler.cc (right):

https://codereview.chromium.org/710183003/diff/80001/components/copresence/ha...
components/copresence/handlers/directive_handler.cc:61: DVLOG(2) << "No
operation associated with directive. Setting op id to "
On 2014/11/10 22:52:54, Rahul Chaturvedi wrote:
> Since this is pretty much expected, make this DVLOG(3).

Done.

[Charlie, 2014-11-10 23:07:10]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-11-10 23:08:04]

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

[commit-bot: I haz the power, 2014-11-11 00:06:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-11 00:07:19]

Patchset 6 (id:??) landed as
https://crrev.com/9baadc9a0fe2f708d553070605d388547f5c3446
Cr-Commit-Position: refs/heads/master@{#303542}