Simulate transient error and verify exponential backoff.

Simulate transient error and verify exponential backoff.

BUG=
TEST=


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

[lipalani1, 2011-07-26 16:10:54]

Test cases to verify exponential backoff algorithm.

Includes client test case changes, fake server changes and unit test for fake
server changes.

Tim tor client and Nick for server !

[Jói, 2011-07-27 12:50:14]

Just a drive-by, no change necessary:  Are you aware of net/base/backoff_entry.h
and .cc which serves as a base class for safe exponential back-off?

Cheers,
Jói

[tim (not reviewing), 2011-07-27 17:02:48]

On 2011/07/27 12:50:14, Jói wrote:
> Just a drive-by, no change necessary:  Are you aware of
net/base/backoff_entry.h
> and .cc which serves as a base class for safe exponential back-off?
> 
> Cheers,
> Jói

Yup :)  It was part of the design doc for our sync scheduler redesign, but the
sync code has implemented it's own exp backoff since M4, and we tried to keep as
many things constant as possible during the major change... but I think akalin@
had reached out a while back about consolidating exponential backoff code here
as well as with the xmpp logic... memory is hazy. But we do want to do this at
some point.

[tim (not reviewing), 2011-07-27 18:14:05]

On 2011/07/27 17:02:48, timsteele wrote:
> On 2011/07/27 12:50:14, Jói wrote:
> > Just a drive-by, no change necessary:  Are you aware of
> net/base/backoff_entry.h
> > and .cc which serves as a base class for safe exponential back-off?
> > 
> > Cheers,
> > Jói
> 
> Yup :)  It was part of the design doc for our sync scheduler redesign, but the
> sync code has implemented it's own exp backoff since M4, and we tried to keep
as
> many things constant as possible during the major change... but I think
akalin@
> had reached out a while back about consolidating exponential backoff code here
> as well as with the xmpp logic... memory is hazy. But we do want to do this at
> some point.

Although you do raise a good point that maybe now is not a bad time to start
doing this transition.  We already use it for HTTP errors because we use
URLFetcher, but maybe we can extend it to our app-specific errors.

I think having tests in place a priori is a fine idea though, so we can likely
proceed here.  But lets file a bug to track.

[Jói, 2011-07-27 23:35:27]

SGTM

On Wed, Jul 27, 2011 at 6:14 PM,  <tim@chromium.org> wrote:
> On 2011/07/27 17:02:48, timsteele wrote:
>>
>> On 2011/07/27 12:50:14, Jói wrote:
>> > Just a drive-by, no change necessary:  Are you aware of
>> net/base/backoff_entry.h
>> > and .cc which serves as a base class for safe exponential back-off?
>> >
>> > Cheers,
>> > Jói
>
>> Yup :)  It was part of the design doc for our sync scheduler redesign, but
>> the
>> sync code has implemented it's own exp backoff since M4, and we tried to
>> keep
>
> as
>>
>> many things constant as possible during the major change... but I think
>
> akalin@
>>
>> had reached out a while back about consolidating exponential backoff code
>> here
>> as well as with the xmpp logic... memory is hazy. But we do want to do
>> this at
>> some point.
>
> Although you do raise a good point that maybe now is not a bad time to start
> doing this transition.  We already use it for HTTP errors because we use
> URLFetcher, but maybe we can extend it to our app-specific errors.
>
> I think having tests in place a priori is a fine idea though, so we can
> likely
> proceed here.  But lets file a bug to track.
>
> http://codereview.chromium.org/7477004/
>

[ncarter (slow), 2011-07-29 18:52:17]

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
net/tools/testserver/chromiumsync.py:102: 
The style guide requires 2 blank lines between top-level definitions (i.e. class
or function defs that aren't inside some other scope).

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
net/tools/testserver/chromiumsync.py:941: """Raises Transiet error if
|transient_error| variable is set."""
"Transiet" is a misspelling, you should probably say "TransientError" to
identify the exact exception.  Also, quoting a variable name with
|transient_error| is not customary good python style.

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
net/tools/testserver/chromiumsync.py:942: if self.transient_error == True:
Just say "if self.transient_error:".  While this is not exactly equivalent to
"== True"  -- it's different on inputs like "aa" or [False] or {1 : [1, "b"]} ,
all of which are True for the purposes of boolean contexts -- the convention in
python is to not check for equality, but instead allow variables to be evaluated
in the boolean context, as this is considered more readable.

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
File net/tools/testserver/chromiumsync_test.py (right):

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
net/tools/testserver/chromiumsync_test.py:617: 
This line needs to come back (top level definition)

[Raghu Simha, 2011-08-05 18:04:52]

Tim suggested I do a drive-by, following which I have a few comments. Please let
me take a look at this after you send it to the sync trybots. Also, it might be
useful to locally test this using the integration tests, with
--vmodule="profile_sync_service_harness=1". This should help verify that the
transitions to and from WAITING_FOR_EXPONENTIAL_BACKOFF_VERIFICATION are working
as designed.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:32: static const int
kExponentialBackoffVerificationTimeoutMs = 60000;
Ideally, you should use the pre-defined values in base/test/test_timeouts.{h/cc}
instead of the above two values. Previously, test_timeouts used to live in
chrome/test, and wasn't available for use by files in chrome/browser (which is
why kLiveSyncOperationTimeoutMs was added above). Now that the files have been
moved to base/test, it might be possible to simply include test_timeouts.h and
use TestTimeouts::live_operation_timeout_ms, etc. Could you please try this out
and see if deps complains?

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:32: static const int
kExponentialBackoffVerificationTimeoutMs = 60000;
Curious: Why 60 seconds and not 45 seconds? (or 90 seconds, for that matter)

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:333: case
WAITING_FOR_EXPONENTIAL_BACKOFF_VERIFICATION: {
VLOG(1) << GetClientInfoString("WAITING_FOR_EXPONENTIAL_BACKOFF_VERIFICATION");

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:337:
EXPONENTIAL_BACKOFF_VERIFICATION_FAILED);
The next state should be one of the "there is nothing to do" states. Perhaps
FULLY_SYNCED.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:342:
EXPONENTIAL_BACKOFF_VERIFICATION_SUCEEDED);
The next state should be one of the "there is nothing to do" states. Perhaps
FULLY_SYNCED.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:346: break;
The break statement goes inside the brace just above it.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:502: return (wait_state_ ==
EXPONENTIAL_BACKOFF_VERIFICATION_SUCEEDED);
return (retry_verifier_.RetryCount() >= 5);

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:197: // The client is in the
exponential back off mode. Verify that backoffs are
s/The client/The sync client/

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:202:
EXPONENTIAL_BACKOFF_VERIFICATION_SUCEEDED,
Get rid of the SUCCEEDED and FAILED states. Other methods in this class that use
WAITING_FOR_* states figure out whether they've succeeded or failed by querying
internal state variables after SignalStateCompleteWithNextState has been called.
If we add 3 states per method, the number of wait states will become intractable
over time.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:279: // Keep track of the
retries for verification.
Perhaps a more descriptive comment like "Keeps track of the number of attempts
at exponential backoff."?

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
File chrome/browser/sync/retry_verifier.h (right):

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:9: 
Delete extra blank line.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:12: 
Delete extra blank line.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:14: namespace sessions {
indent -= 2.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:15: struct SyncSessionSnapshot;
indent -= 4.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:41: DelayInfo delay_table_[kMaxRetry];
DISALLOW_COPY_AND_ASSIGN(RetryVerifier);

http://codereview.chromium.org/7477004/diff/11003/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7477004/diff/11003/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:3542: >>>>>>> fix.
Forgot to merge this?

http://codereview.chromium.org/7477004/diff/11003/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/7477004/diff/11003/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:571: >>>>>>> fix.
Merge.

http://codereview.chromium.org/7477004/diff/11003/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:581: >>>>>>> fix.
Merge.

http://codereview.chromium.org/7477004/diff/11003/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:913: """Raises Transiet error if
|transient_error| variable is set."""
s/Transiet/Transient/

http://codereview.chromium.org/7477004/diff/11003/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:953: >>>>>>> fix.
Merge.

[Raghu Simha, 2011-08-05 18:07:51]

Also, please make sure that the BUG= and TEST= fields in the patch description
are populated. It will greatly help build sheriffs keep track of which test
failures are legit and which ones are bogus, should something break when you
land this.

[lipalani1, 2011-08-05 21:33:55]

Fixed all the comments.
Raghu - please review.

Nick - sorry for not fixing your comments. I was caught up in the production
issue. Can you review the py portion once more. Shouldnt take long?

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
net/tools/testserver/chromiumsync.py:102: 
On 2011/07/29 18:52:17, ncarter wrote:
> The style guide requires 2 blank lines between top-level definitions (i.e.
class
> or function defs that aren't inside some other scope).

Done.

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
net/tools/testserver/chromiumsync.py:941: """Raises Transiet error if
|transient_error| variable is set."""
On 2011/07/29 18:52:17, ncarter wrote:
> "Transiet" is a misspelling, you should probably say "TransientError" to
> identify the exact exception.  Also, quoting a variable name with
> |transient_error| is not customary good python style.

Done.

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
net/tools/testserver/chromiumsync.py:942: if self.transient_error == True:
On 2011/07/29 18:52:17, ncarter wrote:
> Just say "if self.transient_error:".  While this is not exactly equivalent to
> "== True"  -- it's different on inputs like "aa" or [False] or {1 : [1, "b"]}
,
> all of which are True for the purposes of boolean contexts -- the convention
in
> python is to not check for equality, but instead allow variables to be
evaluated
> in the boolean context, as this is considered more readable.

Done.

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
File net/tools/testserver/chromiumsync_test.py (right):

http://codereview.chromium.org/7477004/diff/5001/net/tools/testserver/chromiu...
net/tools/testserver/chromiumsync_test.py:617: 
On 2011/07/29 18:52:17, ncarter wrote:
> This line needs to come back (top level definition)

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:32: static const int
kExponentialBackoffVerificationTimeoutMs = 60000;
If you add up all the max retry values for 4 retries you get close to 45
seconds. So the timeout is 1 minute.
On 2011/08/05 18:04:52, rsimha wrote:
> Curious: Why 60 seconds and not 45 seconds? (or 90 seconds, for that matter)

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:32: static const int
kExponentialBackoffVerificationTimeoutMs = 60000;
Added a todo here.
On 2011/08/05 18:04:52, rsimha wrote:
> Ideally, you should use the pre-defined values in
base/test/test_timeouts.{h/cc}
> instead of the above two values. Previously, test_timeouts used to live in
> chrome/test, and wasn't available for use by files in chrome/browser (which is
> why kLiveSyncOperationTimeoutMs was added above). Now that the files have been
> moved to base/test, it might be possible to simply include test_timeouts.h and
> use TestTimeouts::live_operation_timeout_ms, etc. Could you please try this
out
> and see if deps complains?

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:333: case
WAITING_FOR_EXPONENTIAL_BACKOFF_VERIFICATION: {
On 2011/08/05 18:04:52, rsimha wrote:
> VLOG(1) <<
GetClientInfoString("WAITING_FOR_EXPONENTIAL_BACKOFF_VERIFICATION");

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:337:
EXPONENTIAL_BACKOFF_VERIFICATION_FAILED);
On 2011/08/05 18:04:52, rsimha wrote:
> The next state should be one of the "there is nothing to do" states. Perhaps
> FULLY_SYNCED.

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:342:
EXPONENTIAL_BACKOFF_VERIFICATION_SUCEEDED);
On 2011/08/05 18:04:52, rsimha wrote:
> The next state should be one of the "there is nothing to do" states. Perhaps
> FULLY_SYNCED.

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:346: break;
On 2011/08/05 18:04:52, rsimha wrote:
> The break statement goes inside the brace just above it.

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:502: return (wait_state_ ==
EXPONENTIAL_BACKOFF_VERIFICATION_SUCEEDED);
Done as we discussed.
On 2011/08/05 18:04:52, rsimha wrote:
> return (retry_verifier_.RetryCount() >= 5);

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:197: // The client is in the
exponential back off mode. Verify that backoffs are
On 2011/08/05 18:04:52, rsimha wrote:
> s/The client/The sync client/

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:202:
EXPONENTIAL_BACKOFF_VERIFICATION_SUCEEDED,
On 2011/08/05 18:04:52, rsimha wrote:
> Get rid of the SUCCEEDED and FAILED states. Other methods in this class that
use
> WAITING_FOR_* states figure out whether they've succeeded or failed by
querying
> internal state variables after SignalStateCompleteWithNextState has been
called.
> If we add 3 states per method, the number of wait states will become
intractable
> over time.

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:279: // Keep track of the
retries for verification.
On 2011/08/05 18:04:52, rsimha wrote:
> Perhaps a more descriptive comment like "Keeps track of the number of attempts
> at exponential backoff."?

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
File chrome/browser/sync/retry_verifier.h (right):

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:9: 
On 2011/08/05 18:04:52, rsimha wrote:
> Delete extra blank line.

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:12: 
On 2011/08/05 18:04:52, rsimha wrote:
> Delete extra blank line.

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:14: namespace sessions {
On 2011/08/05 18:04:52, rsimha wrote:
> indent -= 2.

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:15: struct SyncSessionSnapshot;
On 2011/08/05 18:04:52, rsimha wrote:
> indent -= 4.

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:41: DelayInfo delay_table_[kMaxRetry];
On 2011/08/05 18:04:52, rsimha wrote:
> DISALLOW_COPY_AND_ASSIGN(RetryVerifier);

Done.

http://codereview.chromium.org/7477004/diff/11003/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7477004/diff/11003/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:3542: >>>>>>> fix.
On 2011/08/05 18:04:52, rsimha wrote:
> Forgot to merge this?

Done.

http://codereview.chromium.org/7477004/diff/11003/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/7477004/diff/11003/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:571: >>>>>>> fix.
On 2011/08/05 18:04:52, rsimha wrote:
> Merge.

Done.

http://codereview.chromium.org/7477004/diff/11003/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:581: >>>>>>> fix.
On 2011/08/05 18:04:52, rsimha wrote:
> Merge.

Done.

http://codereview.chromium.org/7477004/diff/11003/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:913: """Raises Transiet error if
|transient_error| variable is set."""
On 2011/08/05 18:04:52, rsimha wrote:
> s/Transiet/Transient/

Done.

http://codereview.chromium.org/7477004/diff/11003/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:953: >>>>>>> fix.
On 2011/08/05 18:04:52, rsimha wrote:
> Merge.

Done.

[ncarter (slow), 2011-08-05 22:44:17]

python parts LGTM; 1 nit

http://codereview.chromium.org/7477004/diff/22001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/7477004/diff/22001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:1496: 
Needs another blank line here.

[Raghu Simha, 2011-08-05 22:45:09]

Getting there :)

A few more (mostly minor) comments from me.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:20: #include
"base/test/test_timeouts.h"
Does this need to be included?

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:29: // TODO (rsimha): Remove
the following lines once crbug.com/91863 is fixed.
No space between TODO and open paren. (The "TODOs" link at http://who/me
searches for a specific string.)

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:33: // The amount of time we
wait for testcases that verify exponential backoff.
nit: s/testcases/test cases/

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:338:
retry_verifier_.VerifyRetryInterval(*GetLastSessionSnapshot());
This will segfault if GetLastSessionSnapshot() returns null. Consider adding a
null check.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:367: }
Handle the WAITING_FOR_NOTHING case here. All you need to do is log the state
with a comment, similar to FULLY_SYNCED and SYNC_DISABLED.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:494:
retry_verifier_.Initialize(*GetLastSessionSnapshot());
This will segfault if GetLastSessionSnapshot() returns null. Consider adding a
null check.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:80: // Blocks the caller
until exponential back off has been verified to happen.
nit: s/back off/backoff/

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:201: // The client
verification is complete. We dont care about the state of
nit: s/dont/don't/

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:277: // Keeps track of number
of attempts at exponential backoff and its related
nit: s/number/the number/

[lipalani1, 2011-08-05 23:09:29]

Removing the other reviewers.

Raghu - please review once more.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:20: #include
"base/test/test_timeouts.h"
On 2011/08/05 22:45:09, rsimha wrote:
> Does this need to be included?

Done.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:29: // TODO (rsimha): Remove
the following lines once crbug.com/91863 is fixed.
On 2011/08/05 22:45:09, rsimha wrote:
> No space between TODO and open paren. (The "TODOs" link at http://who/me
> searches for a specific string.)

Done.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:33: // The amount of time we
wait for testcases that verify exponential backoff.
On 2011/08/05 22:45:09, rsimha wrote:
> nit: s/testcases/test cases/

Done.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:338:
retry_verifier_.VerifyRetryInterval(*GetLastSessionSnapshot());
On 2011/08/05 22:45:09, rsimha wrote:
> This will segfault if GetLastSessionSnapshot() returns null. Consider adding a
> null check.

Done.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:367: }
On 2011/08/05 22:45:09, rsimha wrote:
> Handle the WAITING_FOR_NOTHING case here. All you need to do is log the state
> with a comment, similar to FULLY_SYNCED and SYNC_DISABLED.

Done.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:494:
retry_verifier_.Initialize(*GetLastSessionSnapshot());
On 2011/08/05 22:45:09, rsimha wrote:
> This will segfault if GetLastSessionSnapshot() returns null. Consider adding a
> null check.

Done.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:80: // Blocks the caller
until exponential back off has been verified to happen.
On 2011/08/05 22:45:09, rsimha wrote:
> nit: s/back off/backoff/

Done.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:201: // The client
verification is complete. We dont care about the state of
On 2011/08/05 22:45:09, rsimha wrote:
> nit: s/dont/don't/

Done.

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:277: // Keeps track of number
of attempts at exponential backoff and its related
On 2011/08/05 22:45:09, rsimha wrote:
> nit: s/number/the number/

Done.

http://codereview.chromium.org/7477004/diff/22001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/7477004/diff/22001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:1496: 
On 2011/08/05 22:44:25, ncarter wrote:
> Needs another blank line here.

Done.

[Raghu Simha, 2011-08-05 23:23:58]

Changes to chrome/test/live_sync and profile_sync_service_harness LGTM pending a
couple more minor comments and pending green trybot runs.

I'm not an expert on the rest of the changes in chrome/browser/sync. Perhaps
have tim@ or zea@ look at them?

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7477004/diff/22001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:494:
retry_verifier_.Initialize(*GetLastSessionSnapshot());
On 2011/08/05 23:09:29, lipalani1 wrote:
> On 2011/08/05 22:45:09, rsimha wrote:
> > This will segfault if GetLastSessionSnapshot() returns null. Consider adding
a
> > null check.
> 
> Done.

Still need a null check here.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:373: return
original_wait_state != wait_state_;
s/WAIT_FOR_NOTHING/WAITING_FOR_NOTHING/

[lipalani1, 2011-08-08 16:22:42]

Nicolas - can you go over the rest of the changes under chrome/browser/sync.

[lipalani1, 2011-08-08 20:32:30]

Tim -
Raghu has gone through all the test changes including the PSSH and other allied
helpers. Can you go through the sync changes in the following files?
chrome/browser/sync/* except the PSSH.

these changes should be small as all they do is add a variable called sync start
time.

[tim (not reviewing), 2011-08-08 23:11:35]

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
File chrome/browser/sync/retry_verifier.cc (right):

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.cc:15: DelayInfo CalculateDelay(int64
current_delay) {
this should be in a namespace (possibly even anon namespace inside
browser_sync). I'm surprised this doesn't collide with something in the global
ns already

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.cc:56: // amount of time. The global timeout
is defined in PSH as
don't use acronyms in comments unless the acronym is the more common term.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
File chrome/browser/sync/retry_verifier.h (right):

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:15: }
}  // namespace browser_sync

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:26: class RetryVerifier {
something as common-sounding yet specific as this should be in a namespace (at
least browser_sync::)

It might be better to also tweak the name... HarnessRetryVerifier? RetryVerifier
is a pretty open ended name.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:31: int RetryCount() const { return
retry_count_; }
style guide says: retry_count()

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:32: void Initialize(const
browser_sync::sessions::SyncSessionSnapshot& snap);
what is this param for? (comment)

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:35: bool Done() const { return done_; }
style guide says: "done()"

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:36: bool Success() const { return Done() &&
success_; }
DidSucceed or Succeeded

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/session...
File chrome/browser/sync/sessions/status_controller.h (right):

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/session...
chrome/browser/sync/sessions/status_controller.h:196: base::Time
sync_start_time() const {
Comment what "start" actually means.

[lipalani1, 2011-08-09 01:39:01]

Please review once more.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:373: VLOG(1) <<
GetClientInfoString("WAIT_FOR_NOTHING");
On 2011/08/05 23:23:58, rsimha wrote:
> s/WAIT_FOR_NOTHING/WAITING_FOR_NOTHING/

Done.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
File chrome/browser/sync/retry_verifier.cc (right):

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.cc:15: DelayInfo CalculateDelay(int64
current_delay) {
On 2011/08/08 23:11:35, timsteele wrote:
> this should be in a namespace (possibly even anon namespace inside
> browser_sync). I'm surprised this doesn't collide with something in the global
> ns already

Done.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.cc:56: // amount of time. The global timeout
is defined in PSH as
On 2011/08/08 23:11:35, timsteele wrote:
> don't use acronyms in comments unless the acronym is the more common term.

Done.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
File chrome/browser/sync/retry_verifier.h (right):

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:15: }
On 2011/08/08 23:11:35, timsteele wrote:
> }  // namespace browser_sync

Done.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:26: class RetryVerifier {
Kept the name but put a namespace called sync_test_harness.
On 2011/08/08 23:11:35, timsteele wrote:
> something as common-sounding yet specific as this should be in a namespace (at
> least browser_sync::)
> 
> It might be better to also tweak the name... HarnessRetryVerifier?
RetryVerifier
> is a pretty open ended name.

Done.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:31: int RetryCount() const { return
retry_count_; }
On 2011/08/08 23:11:35, timsteele wrote:
> style guide says: retry_count()

Done.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:32: void Initialize(const
browser_sync::sessions::SyncSessionSnapshot& snap);
added a comment.
On 2011/08/08 23:11:35, timsteele wrote:
> what is this param for? (comment)

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:35: bool Done() const { return done_; }
On 2011/08/08 23:11:35, timsteele wrote:
> style guide says: "done()"

Done.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:36: bool Success() const { return Done() &&
success_; }
On 2011/08/08 23:11:35, timsteele wrote:
> DidSucceed or Succeeded

Done.

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/session...
File chrome/browser/sync/sessions/status_controller.h (right):

http://codereview.chromium.org/7477004/diff/20004/chrome/browser/sync/session...
chrome/browser/sync/sessions/status_controller.h:196: base::Time
sync_start_time() const {
On 2011/08/08 23:11:35, timsteele wrote:
> Comment what "start" actually means.

Done.

[Raghu Simha, 2011-08-09 04:28:05]

http://codereview.chromium.org/7477004/diff/30001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7477004/diff/30001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:502:
retry_verifier_.Initialize(*GetLastSessionSnapshot());
On 2011/08/05 23:23:58, rsimha wrote:
> On 2011/08/05 23:09:29, lipalani1 wrote:
> > On 2011/08/05 22:45:09, rsimha wrote:
> > > This will segfault if GetLastSessionSnapshot() returns null. Consider
adding
> a
> > > null check.
> > 
> > Done.
> 
> Still need a null check here.

Repeating my earlier comment: Still need a null check here :)

[lipalani1, 2011-08-09 19:47:33]

Tim - Please review.
Fixed Raghu's feedback as well.

[tim (not reviewing), 2011-08-09 19:57:47]

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer.cc:119:
session->status_controller()->set_syncing(true);
Why are we making this change?  What do you mean by "would call
set_syncing(false)"?  We don't always go through to SYNCER_END from
DOWNLOAD_UPDATES (e.g. config) and probably shouldn't make assumptions like that
given that callers can invoke SyncShare with arbitrary start/stop states.

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/retry_v...
File chrome/browser/sync/retry_verifier.cc (right):

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.cc:39: void FillDelayTable(DelayInfo*
delay_table, int count) {
put this in same ns as above

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/retry_v...
File chrome/browser/sync/retry_verifier.h (right):

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:17: namespace sync_test_harness {
a namespace that doesn't match the directory hierarchy like this is against the
style guide as well.  I guess lets put it in browser_sync and call it done.

[lipalani1, 2011-08-09 20:36:48]

Tim - please review.

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer.cc:119:
session->status_controller()->set_syncing(true);
hey even in that case we would always call syncer_end. otherwise the snapshot
wont be send. Thats why I thought it was safe to put it. But now you bring it up
I am going with my original idea which is to make it a scoped variable so sync
end and start would be called correctly.
Earlier sync start and sync end was kind of broken. we would call
set_syncing(true) when processing updates and again in upload commits. and call
stop syncing in syncerend. 
With this change whenever we are in syncer we would call set_syncing(true) and
when we exit we would call set_syncing(false). this includes clearing of
disabled types.
On 2011/08/09 19:57:47, timsteele wrote:
> Why are we making this change?  What do you mean by "would call
> set_syncing(false)"?  We don't always go through to SYNCER_END from
> DOWNLOAD_UPDATES (e.g. config) and probably shouldn't make assumptions like
that
> given that callers can invoke SyncShare with arbitrary start/stop states.

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/retry_v...
File chrome/browser/sync/retry_verifier.cc (right):

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.cc:39: void FillDelayTable(DelayInfo*
delay_table, int count) {
On 2011/08/09 19:57:47, timsteele wrote:
> put this in same ns as above

Done.

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/retry_v...
File chrome/browser/sync/retry_verifier.h (right):

http://codereview.chromium.org/7477004/diff/32001/chrome/browser/sync/retry_v...
chrome/browser/sync/retry_verifier.h:17: namespace sync_test_harness {
ah.. changed that.
On 2011/08/09 19:57:47, timsteele wrote:
> a namespace that doesn't match the directory hierarchy like this is against
the
> style guide as well.  I guess lets put it in browser_sync and call it done.

[lipalani1, 2011-08-09 23:59:55]

Made it SetSyncInProgressAndUpdateStartTime.
Please review!

[tim (not reviewing), 2011-08-10 00:08:29]

Please address comments, and then this LGTM, including changes you make in
response.

http://codereview.chromium.org/7477004/diff/2003/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/7477004/diff/2003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer.cc:129: // going to SYNCER_END because of
various cleanup steps.
remove comment

http://codereview.chromium.org/7477004/diff/2003/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer.h (right):

http://codereview.chromium.org/7477004/diff/2003/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer.h:95: sessions::SyncSession* session_;
disallow copy/assign

http://codereview.chromium.org/7477004/diff/2003/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7477004/diff/2003/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:33: static const int
kExponentialBackoffVerificationTimeoutMs = 60000;
couldn't we use the TestTimeouts value rather than make the problem worse? 
there's a 60000 value already...

http://codereview.chromium.org/7477004/diff/2003/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:341: CHECK(snap);
the deref below is probably more useful than this check?

http://codereview.chromium.org/7477004/diff/2003/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:344:
SignalStateCompleteWithNextState(WAITING_FOR_NOTHING);
nit - indent

[commit-bot: I haz the power, 2011-08-10 23:30:16]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux, revision is 96242, job name
was 7477004-42001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-08-11 01:22:34]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux, revision is 96292, job name
was 7477004-42001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-08-12 05:37:50]

Change committed as 96515