Remove broken variables from sync's AllStatus

Remove broken variables from sync's AllStatus

* syncer_stuck: This value would get set if we failed to make progress
in resolving conflict sets.  The conflict set processing code is never
executed (crbug.com/107816).  I suspect there were other problems with
this status indicator, but this alone is enough to justify its removal.

* server_broken: Because the number of commits was never greater than
25, there's no way we could see >100 commits bounce because of a
transient error in a single sync cycle.  This value is always false.

* disk_full: Never set.  Always false.

* CONFLICT status summary value: It depends on syncer_stuck.

* HandleAuthWatcherEvent(): This method was undefined.

This is not the full set of AllStatus fixes.  There are some other
status indicators that should be removed, but I wanted to limit the
scope of this commit to removing broken indicators.  Mostly useless but
still functional indicators will be removed in future commits.

BUG=98346, 105739
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=118499

[rlarocque, 2012-01-10 00:07:42]

Please review.

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
File chrome/browser/sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
chrome/browser/sync/protocol/client_debug_info.proto:17: // optional bool
syncer_stuck = 1; // Was always false, now obsolete.
This is my preferred mechanism for deprecating protobuf fields.  If you know of
a better way to handle it, let me know.

[tim (not reviewing), 2012-01-10 17:01:47]

+Albert / Raz, question of curiosity about protobuf options in
client_debug_info.proto

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncer_unittest.cc (left):

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_unittest.cc:1702: TEST_F(SyncerTest,
DontGetStuckWithTwoSameNames) {
Is it bad to test that two updates with the same name doesn't hose sync? Can we
retrofit this test to make it useful?

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_unittest.cc:3271:
EXPECT_FALSE(session_->status_controller().syncer_status().syncer_stuck);
I kind of have the same question as above.  I don't think the spirit of this
test is obsolete.

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
File chrome/browser/sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
chrome/browser/sync/protocol/client_debug_info.proto:17: // optional bool
syncer_stuck = 1; // Was always false, now obsolete.
On 2012/01/10 00:07:43, rlarocque wrote:
> This is my preferred mechanism for deprecating protobuf fields.  If you know
of
> a better way to handle it, let me know.

I really wonder if the spirit of a "stuck" signal is obsolete.  We were
mis-reporting it, but it's not like we haven't had reports of "stuck" like
behavior.  Are you absolutely certain it would never be useful to send this as
debug info to the server?  I'm not.  The cost of an extra .proto field isn't
that great.  We could add the deprecated=true option (does nothing in C++ but
generates @Deprecated annotation + warnings in java), I'm not sure if that is
more or less reversible than deleting altogether... perhaps Raz or Albert can
comment.

[rlarocque, 2012-01-10 21:39:32]

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncer_unittest.cc (left):

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_unittest.cc:1702: TEST_F(SyncerTest,
DontGetStuckWithTwoSameNames) {
On 2012/01/10 17:01:47, timsteele wrote:
> Is it bad to test that two updates with the same name doesn't hose sync? Can
we
> retrofit this test to make it useful?

My understanding is that we don't actually look at the names of items a whole
lot, even in bookrmarks code.  Also keep in mind this test is configured with
"passive" workers, so no data type specific code is being run.

This test also makes assumptions about the way SyncShare is invoked, which I
think is a mistake.  We've seen that the HasMoreToSync() method and
SyncScheduler's loop to run multiple sync cycles has a huge impact on system
behaviour.  This test bypasses that code.

I'm not sure there's a lot of value in this test if we look at it from a "white
box" perspective.  Could it be made useful?  Maybe.  Is it worth the effort? 
Probably not.

I suspect that most of the tests in this file are not very useful.  But we
haven't had any reason to delete them yet, so they live on.

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_unittest.cc:3271:
EXPECT_FALSE(session_->status_controller().syncer_status().syncer_stuck);
On 2012/01/10 17:01:47, timsteele wrote:
> I kind of have the same question as above.  I don't think the spirit of this
> test is obsolete.

I'm not sure this ever made sense.  What did we hope to prove with 4000 nested
directories that can't be proven with 10?  Why was there a risk that the syncer
could get stuck in this case, event when syncer_stuck actually meant something?

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
File chrome/browser/sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
chrome/browser/sync/protocol/client_debug_info.proto:17: // optional bool
syncer_stuck = 1; // Was always false, now obsolete.
On 2012/01/10 17:01:47, timsteele wrote:
> On 2012/01/10 00:07:43, rlarocque wrote:
> > This is my preferred mechanism for deprecating protobuf fields.  If you know
> of
> > a better way to handle it, let me know.
> 
> I really wonder if the spirit of a "stuck" signal is obsolete.  We were
> mis-reporting it, but it's not like we haven't had reports of "stuck" like
> behavior.  Are you absolutely certain it would never be useful to send this as
> debug info to the server?  I'm not.  The cost of an extra .proto field isn't
> that great.  We could add the deprecated=true option (does nothing in C++ but
> generates @Deprecated annotation + warnings in java), I'm not sure if that is
> more or less reversible than deleting altogether... perhaps Raz or Albert can
> comment.

Even if we revive the concept of "syncer_stuck" (I hope we don't), it will be
based on significantly different logic than the current variable.  They should
be named differently, so we don't have to research whether or not the flag is
broken in a particular version of Chrome.

[tim (not reviewing), 2012-01-11 02:15:35]

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncer_unittest.cc (left):

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_unittest.cc:1702: TEST_F(SyncerTest,
DontGetStuckWithTwoSameNames) {
On 2012/01/10 21:39:32, rlarocque wrote:
> On 2012/01/10 17:01:47, timsteele wrote:
> > Is it bad to test that two updates with the same name doesn't hose sync? Can
> we
> > retrofit this test to make it useful?
> 
> My understanding is that we don't actually look at the names of items a whole
> lot, even in bookrmarks code.  Also keep in mind this test is configured with
> "passive" workers, so no data type specific code is being run.
> 
> This test also makes assumptions about the way SyncShare is invoked, which I
> think is a mistake.  We've seen that the HasMoreToSync() method and
> SyncScheduler's loop to run multiple sync cycles has a huge impact on system
> behaviour.  This test bypasses that code.
> 
> I'm not sure there's a lot of value in this test if we look at it from a
"white
> box" perspective.  Could it be made useful?  Maybe.  Is it worth the effort? 
> Probably not.
> 
> I suspect that most of the tests in this file are not very useful.  But we
> haven't had any reason to delete them yet, so they live on.

That's not true, actually. We haven't deleted them because they are the only
tests that actually exercise the syncer commands together :). I think there is
use to that, whether or not they represent a complete integration test of
exactly the behavior in practice. I think the assumptions being made or hacks
around SyncShare are there because that code isn't the target of these tests.
The target is really various bits of code and branches in syncer commands and
syncer_util that maintain generic integrity of the full tree of data and not the
mechanics of running a session. Incidentally, it wasn't ever the intent of
having data-type specific tests here since we have yet to implement data type
specific conflict resolution at this layer (though implicitly a lot of the cases
here may be biased towards bookmarks due to hierarchical relationships).

If we find otherwise, and that no test in here is currently effectively testing
what it once was (or those things are all irrelevant now), then it should become
our top priority to test the code we have. There are things tested in here that
can no longer happen in practice, but I doubt that is the case for every single
test.

The main argument I would make as to why it is worth having a test covering what
this was intending to cover is because the code is still there.  If we rip out
all code related to names, then we should rip out the test.  But if we have code
that we just stop testing and leave to fester, all of a sudden that path that
you didn't care about can spiral out of control and cause harm.

I wont absolutely insist, but is it really that hard to keep the test working?
Could we not check that things don't wind up in conflict? That a subsequent
SyncShare works as expected?

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_unittest.cc:3271:
EXPECT_FALSE(session_->status_controller().syncer_status().syncer_stuck);
On 2012/01/10 21:39:32, rlarocque wrote:
> On 2012/01/10 17:01:47, timsteele wrote:
> > I kind of have the same question as above.  I don't think the spirit of this
> > test is obsolete.
> 
> I'm not sure this ever made sense.  What did we hope to prove with 4000 nested
> directories that can't be proven with 10?  Why was there a risk that the
syncer
> could get stuck in this case, event when syncer_stuck actually meant
something?

Well, even though I closed the bug myself with a similar argument, look at bug
20185. We have people with ~1000 bookmarks, so I don't think it is ridiculous to
have a test that would test *that* (4k may be excessive) and doesn't either run
out of memory or time out or fail for some strange batching bug or who knows
what.

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
File chrome/browser/sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
chrome/browser/sync/protocol/client_debug_info.proto:17: // optional bool
syncer_stuck = 1; // Was always false, now obsolete.
On 2012/01/10 21:39:32, rlarocque wrote:
> On 2012/01/10 17:01:47, timsteele wrote:
> > On 2012/01/10 00:07:43, rlarocque wrote:
> > > This is my preferred mechanism for deprecating protobuf fields.  If you
know
> > of
> > > a better way to handle it, let me know.
> > 
> > I really wonder if the spirit of a "stuck" signal is obsolete.  We were
> > mis-reporting it, but it's not like we haven't had reports of "stuck" like
> > behavior.  Are you absolutely certain it would never be useful to send this
as
> > debug info to the server?  I'm not.  The cost of an extra .proto field isn't
> > that great.  We could add the deprecated=true option (does nothing in C++
but
> > generates @Deprecated annotation + warnings in java), I'm not sure if that
is
> > more or less reversible than deleting altogether... perhaps Raz or Albert
can
> > comment.
> 
> Even if we revive the concept of "syncer_stuck" (I hope we don't), it will be
> based on significantly different logic than the current variable.  They should
> be named differently, so we don't have to research whether or not the flag is
> broken in a particular version of Chrome.

It's just that we have made no change that ensures a client will never become
"stuck". And we only added this bit here a few months ago and I don't think
we've used it a single time yet (thankfully we haven't had any major prod issues
to debug!), so I don't think there is much legacy convention to worry about. And
lastly, hopes and wishes aside, I think it's safe to say this concept will
always be around, unless we figure out how to achieve complete perfection even
in the face of cosmic rays :)

But anyway, I'm okay with removing it.  I'd be interested to hear Lingesh's
thoughts (do we want a bug to track sending up something more useful?)

[rlarocque, 2012-01-11 19:58:36]

Comments updated.  We should continue the discussion of testing offline.

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncer_unittest.cc (left):

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_unittest.cc:1702: TEST_F(SyncerTest,
DontGetStuckWithTwoSameNames) {
On 2012/01/11 02:15:35, timsteele wrote:
> On 2012/01/10 21:39:32, rlarocque wrote:
> > On 2012/01/10 17:01:47, timsteele wrote:
> > > Is it bad to test that two updates with the same name doesn't hose sync?
Can
> > we
> > > retrofit this test to make it useful?
> > 
> > My understanding is that we don't actually look at the names of items a
whole
> > lot, even in bookrmarks code.  Also keep in mind this test is configured
with
> > "passive" workers, so no data type specific code is being run.
> > 
> > This test also makes assumptions about the way SyncShare is invoked, which I
> > think is a mistake.  We've seen that the HasMoreToSync() method and
> > SyncScheduler's loop to run multiple sync cycles has a huge impact on system
> > behaviour.  This test bypasses that code.
> > 
> > I'm not sure there's a lot of value in this test if we look at it from a
> "white
> > box" perspective.  Could it be made useful?  Maybe.  Is it worth the effort?

> > Probably not.
> > 
> > I suspect that most of the tests in this file are not very useful.  But we
> > haven't had any reason to delete them yet, so they live on.
> 
> That's not true, actually. We haven't deleted them because they are the only
> tests that actually exercise the syncer commands together :). I think there is
> use to that, whether or not they represent a complete integration test of
> exactly the behavior in practice. I think the assumptions being made or hacks
> around SyncShare are there because that code isn't the target of these tests.
> The target is really various bits of code and branches in syncer commands and
> syncer_util that maintain generic integrity of the full tree of data and not
the
> mechanics of running a session. Incidentally, it wasn't ever the intent of
> having data-type specific tests here since we have yet to implement data type
> specific conflict resolution at this layer (though implicitly a lot of the
cases
> here may be biased towards bookmarks due to hierarchical relationships).
> 
> If we find otherwise, and that no test in here is currently effectively
testing
> what it once was (or those things are all irrelevant now), then it should
become
> our top priority to test the code we have. There are things tested in here
that
> can no longer happen in practice, but I doubt that is the case for every
single
> test.
> 
> The main argument I would make as to why it is worth having a test covering
what
> this was intending to cover is because the code is still there.  If we rip out
> all code related to names, then we should rip out the test.  But if we have
code
> that we just stop testing and leave to fester, all of a sudden that path that
> you didn't care about can spiral out of control and cause harm.
> 
> I wont absolutely insist, but is it really that hard to keep the test working?
> Could we not check that things don't wind up in conflict? That a subsequent
> SyncShare works as expected?

I exaggerated when I commented on the state of this file.  But I really do think
that this particular test serves no purpose.  

As far as I can tell, this test is trying to test the case where two new entries
have the same NON_UNIQUE_NAME and different IDs.  

I grep'ed for NON_UNIQUE_NAME and found that it plays a minor role in conflict
resolution, which won't be executed in this case because these two entries do
not conflict.  All other references to NON_UNIQUE_NAME seem to be in simple
getter/setter methods, functions that copy server fields to client fields and
vice-versa, etc.  I don't see anywhere in the syncer code where we compare the
(SERVER_)NON_UNIQUE_NAMEs of two different entries.

If that's the case, any brokenness that could be detected by this test would
also be detected by any other test that grabs a two sibling updates from the
server and applies them by manually calling SyncShare.

To answer your question, no, it's not hard to keep this test working.  I just
don't see any reason why we should.

See also: crbug.com/107816, which explains why syncer_stuck was always false in
practice and which makes obsolete the current definition of
"SyncRepeatedlyToTriggerStuckSignal()".

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_unittest.cc:3271:
EXPECT_FALSE(session_->status_controller().syncer_status().syncer_stuck);
On 2012/01/11 02:15:35, timsteele wrote:
> On 2012/01/10 21:39:32, rlarocque wrote:
> > On 2012/01/10 17:01:47, timsteele wrote:
> > > I kind of have the same question as above.  I don't think the spirit of
this
> > > test is obsolete.
> > 
> > I'm not sure this ever made sense.  What did we hope to prove with 4000
nested
> > directories that can't be proven with 10?  Why was there a risk that the
> syncer
> > could get stuck in this case, event when syncer_stuck actually meant
> something?
> 
> Well, even though I closed the bug myself with a similar argument, look at bug
> 20185. We have people with ~1000 bookmarks, so I don't think it is ridiculous
to
> have a test that would test *that* (4k may be excessive) and doesn't either
run
> out of memory or time out or fail for some strange batching bug or who knows
> what.

I agree, scale* tests are useful.  But a scale test does not belong among our
unit tests.  Unit tests should aim for good coverage of small areas of code and
fast execution time.

A scale test written in this way is likely to be ineffective.  What if the scale
issue is located in the model association code?  An integration test would be
much more likely to uncover scale issues.  Even better would be to rewrite this
as a performance test, which would make it easier to track performance
regressions in addition to verifying that this scenario does not trigger a
crash.

* scale, in this case, refers scale on the client side as opposed to our usual
definition.

[tim (not reviewing), 2012-01-11 22:27:04]

We chatted offline, LGTM

[tim (not reviewing), 2012-01-11 22:28:15]

(Would still be good for Lingesh to take a look, however)

On Wed, Jan 11, 2012 at 2:27 PM, <tim@chromium.org> wrote:

> We chatted offline, LGTM
>
>
http://codereview.chromium.**org/9149017/<http://codereview.chromium.org/9149...
>

[lipalani1, 2012-01-12 23:44:26]

Sorry for the delay. Just getting back on track. You can expect feedback on both
your reviews in the next 2 hours.
On 2012/01/11 22:28:15, timsteele wrote:
> (Would still be good for Lingesh to take a look, however)
> 
> On Wed, Jan 11, 2012 at 2:27 PM, <mailto:tim@chromium.org> wrote:
> 
> > We chatted offline, LGTM
> >
> >
>
http://codereview.chromium.**org/9149017/%3Chttp://codereview.chromium.org/91...>
> >

[lipalani1, 2012-01-12 23:55:48]

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
File chrome/browser/sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
chrome/browser/sync/protocol/client_debug_info.proto:17: // optional bool
syncer_stuck = 1; // Was always false, now obsolete.
I guess since this is a brand new feature there is not much to worry about.
Since this field is not calculated I am OK deprecating it. and when we have the
new way to calculate the stuck behaviour we could simply use a new field without
worrying about legacy.

As far as the meta question goes: I too think stuck is a useful state to track.
How about we upload 'x', the number of  sync cycles that failed consecutively up
to this point. we track that and upload the x? Which in most cases should be 0
or 1.
If we upload this then from the server side we could do about:sync page
displaying clients with high numbers for x and do some digging there.
On 2012/01/11 02:15:35, timsteele wrote:
> On 2012/01/10 21:39:32, rlarocque wrote:
> > On 2012/01/10 17:01:47, timsteele wrote:
> > > On 2012/01/10 00:07:43, rlarocque wrote:
> > > > This is my preferred mechanism for deprecating protobuf fields.  If you
> know
> > > of
> > > > a better way to handle it, let me know.
> > > 
> > > I really wonder if the spirit of a "stuck" signal is obsolete.  We were
> > > mis-reporting it, but it's not like we haven't had reports of "stuck" like
> > > behavior.  Are you absolutely certain it would never be useful to send
this
> as
> > > debug info to the server?  I'm not.  The cost of an extra .proto field
isn't
> > > that great.  We could add the deprecated=true option (does nothing in C++
> but
> > > generates @Deprecated annotation + warnings in java), I'm not sure if that
> is
> > > more or less reversible than deleting altogether... perhaps Raz or Albert
> can
> > > comment.
> > 
> > Even if we revive the concept of "syncer_stuck" (I hope we don't), it will
be
> > based on significantly different logic than the current variable.  They
should
> > be named differently, so we don't have to research whether or not the flag
is
> > broken in a particular version of Chrome.
> 
> It's just that we have made no change that ensures a client will never become
> "stuck". And we only added this bit here a few months ago and I don't think
> we've used it a single time yet (thankfully we haven't had any major prod
issues
> to debug!), so I don't think there is much legacy convention to worry about.
And
> lastly, hopes and wishes aside, I think it's safe to say this concept will
> always be around, unless we figure out how to achieve complete perfection even
> in the face of cosmic rays :)
> 
> But anyway, I'm okay with removing it.  I'd be interested to hear Lingesh's
> thoughts (do we want a bug to track sending up something more useful?)
>

[albertb, 2012-01-13 15:47:02]

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
File chrome/browser/sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
chrome/browser/sync/protocol/client_debug_info.proto:17: // optional bool
syncer_stuck = 1; // Was always false, now obsolete.
On 2012/01/12 23:55:49, lipalani1 wrote:
> I guess since this is a brand new feature there is not much to worry about.
> Since this field is not calculated I am OK deprecating it. and when we have
the
> new way to calculate the stuck behaviour we could simply use a new field
without
> worrying about legacy.
> 
> As far as the meta question goes: I too think stuck is a useful state to
track.
> How about we upload 'x', the number of  sync cycles that failed consecutively
up
> to this point. we track that and upload the x? Which in most cases should be 0
> or 1.
> If we upload this then from the server side we could do about:sync page
> displaying clients with high numbers for x and do some digging there.
> On 2012/01/11 02:15:35, timsteele wrote:
> > On 2012/01/10 21:39:32, rlarocque wrote:
> > > On 2012/01/10 17:01:47, timsteele wrote:
> > > > On 2012/01/10 00:07:43, rlarocque wrote:
> > > > > This is my preferred mechanism for deprecating protobuf fields.  If
you
> > know
> > > > of
> > > > > a better way to handle it, let me know.
> > > > 
> > > > I really wonder if the spirit of a "stuck" signal is obsolete.  We were
> > > > mis-reporting it, but it's not like we haven't had reports of "stuck"
like
> > > > behavior.  Are you absolutely certain it would never be useful to send
> this
> > as
> > > > debug info to the server?  I'm not.  The cost of an extra .proto field
> isn't
> > > > that great.  We could add the deprecated=true option (does nothing in
C++
> > but
> > > > generates @Deprecated annotation + warnings in java), I'm not sure if
that
> > is
> > > > more or less reversible than deleting altogether... perhaps Raz or
Albert
> > can
> > > > comment.
> > > 
> > > Even if we revive the concept of "syncer_stuck" (I hope we don't), it will
> be
> > > based on significantly different logic than the current variable.  They
> should
> > > be named differently, so we don't have to research whether or not the flag
> is
> > > broken in a particular version of Chrome.
> > 
> > It's just that we have made no change that ensures a client will never
become
> > "stuck". And we only added this bit here a few months ago and I don't think
> > we've used it a single time yet (thankfully we haven't had any major prod
> issues
> > to debug!), so I don't think there is much legacy convention to worry about.
> And
> > lastly, hopes and wishes aside, I think it's safe to say this concept will
> > always be around, unless we figure out how to achieve complete perfection
even
> > in the face of cosmic rays :)
> > 
> > But anyway, I'm okay with removing it.  I'd be interested to hear Lingesh's
> > thoughts (do we want a bug to track sending up something more useful?)
> > 
> 
[deprecated=true] is fine for the server. We'll get a warning if we reference
the field.

[rlarocque, 2012-01-13 22:36:11]

On 2012/01/12 23:55:48, lipalani1 wrote:
>
http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
> File chrome/browser/sync/protocol/client_debug_info.proto (right):
> 
>
http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
> chrome/browser/sync/protocol/client_debug_info.proto:17: // optional bool
> syncer_stuck = 1; // Was always false, now obsolete.
> I guess since this is a brand new feature there is not much to worry about.
> Since this field is not calculated I am OK deprecating it. and when we have
the
> new way to calculate the stuck behaviour we could simply use a new field
without
> worrying about legacy.
> 
> As far as the meta question goes: I too think stuck is a useful state to
track.
> How about we upload 'x', the number of  sync cycles that failed consecutively
up
> to this point. we track that and upload the x? Which in most cases should be 0
> or 1.
> If we upload this then from the server side we could do about:sync page
> displaying clients with high numbers for x and do some digging there.

OK, but could we punt this task to a future commit/bug?  I don't want to add any
new functionality in this commit.  I'm only looking to remove stuff that is
already broken.

I also disagree that such a counter will be useful, though that may be an issue
of terminology.  I'm not sure how you would define "consecutive sync cycle" and
"sync cycle failure".  Currently, the only sync cycle failures we can detect
directly are DIRECTORY_LOOKUP_FAILURES (which never happen) and cases where the
server returns an error code.  In the latter case, can't we rely on the server
for debugging this?  Why should the client upload information that is already
known to the server?

In other words, I'm wondering how the consecutive sync cycle failure counter
would contain data not already known on the server side.

[rlarocque, 2012-01-18 21:30:28]

On 2012/01/13 22:36:11, rlarocque wrote:
> On 2012/01/12 23:55:48, lipalani1 wrote:
> >
>
http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
> > File chrome/browser/sync/protocol/client_debug_info.proto (right):
> > 
> >
>
http://codereview.chromium.org/9149017/diff/1/chrome/browser/sync/protocol/cl...
> > chrome/browser/sync/protocol/client_debug_info.proto:17: // optional bool
> > syncer_stuck = 1; // Was always false, now obsolete.
> > I guess since this is a brand new feature there is not much to worry about.
> > Since this field is not calculated I am OK deprecating it. and when we have
> the
> > new way to calculate the stuck behaviour we could simply use a new field
> without
> > worrying about legacy.
> > 
> > As far as the meta question goes: I too think stuck is a useful state to
> track.
> > How about we upload 'x', the number of  sync cycles that failed
consecutively
> up
> > to this point. we track that and upload the x? Which in most cases should be
0
> > or 1.
> > If we upload this then from the server side we could do about:sync page
> > displaying clients with high numbers for x and do some digging there.
> 
> OK, but could we punt this task to a future commit/bug?  I don't want to add
any
> new functionality in this commit.  I'm only looking to remove stuff that is
> already broken.
> 
> I also disagree that such a counter will be useful, though that may be an
issue
> of terminology.  I'm not sure how you would define "consecutive sync cycle"
and
> "sync cycle failure".  Currently, the only sync cycle failures we can detect
> directly are DIRECTORY_LOOKUP_FAILURES (which never happen) and cases where
the
> server returns an error code.  In the latter case, can't we rely on the server
> for debugging this?  Why should the client upload information that is already
> known to the server?
> 
> In other words, I'm wondering how the consecutive sync cycle failure counter
> would contain data not already known on the server side.

Reviving this.

Lingesh: Are you OK with my suggestion to handle counting sync cycles in a
future commit?

Albert: Are you saying you would *prefer* to use [deprecated=true], rather than
eliminating the variable altogether?  Your previous comment didn't seem to
recommend one option over the other.

[albertb, 2012-01-18 21:36:19]

On 2012/01/18 21:30:28, rlarocque wrote:
> Albert: Are you saying you would *prefer* to use [deprecated=true], rather
than
> eliminating the variable altogether?  Your previous comment didn't seem to
> recommend one option over the other.
No preference from our side. Just let me know when you submit this and I'll
update the server protos.

[lipalani1, 2012-01-18 21:53:49]

Absolutely :) LGTM from my side.
On 2012/01/18 21:36:19, albertb wrote:
> On 2012/01/18 21:30:28, rlarocque wrote:
> > Albert: Are you saying you would *prefer* to use [deprecated=true], rather
> than
> > eliminating the variable altogether?  Your previous comment didn't seem to
> > recommend one option over the other.
> No preference from our side. Just let me know when you submit this and I'll
> update the server protos.

[commit-bot: I haz the power, 2012-01-20 19:31:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9149017/14003

[commit-bot: I haz the power, 2012-01-20 19:33:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9149017/14003

[commit-bot: I haz the power, 2012-01-20 21:17:49]

Change committed as 118499