Sync commit errors should temporarily re-enable trigger pre-commit getupdates

Sync commit errors should temporarily re-enable
trigger pre-commit getupdates

Add a boolean variable in class DataTypeTracker, so every
time when a data type got conflict response from server
during commit, will set that boolean variable to true, then
next time sync, GetUpdate will check the boolean to see if
need to GetUpdate to resolve conflict locally.

BUG=324893
Committed: https://crrev.com/21f43c5af27e24c34565df26bb51fcc704c97597
Cr-Commit-Position: refs/heads/master@{#315339}

Committed: https://crrev.com/f951166bde5650de692d30c542e320a1e6c0e75a
Cr-Commit-Position: refs/heads/master@{#315828}

[Gang Wu, 2015-02-06 22:14:09]

gangwu@chromium.org changed reviewers:
+ maniscalco@chromium.org, stanisc@chromium.org, zea@chromium.org

[Gang Wu, 2015-02-06 22:14:10]

please take a look.

[Nicolas Zea, 2015-02-06 22:37:00]

zea@chromium.org changed reviewers:
+ pvalenzuela@chromium.org

[Nicolas Zea, 2015-02-06 22:37:01]

Nice job! I only have a couple nits, and a request: I think it would be useful
to test this in an integration test using the FakeServer. +Patrick, do we
already have the ability for the fake server to return a per-datatype conflict
response?

https://codereview.chromium.org/905853002/diff/1/sync/engine/syncer_unittest.cc
File sync/engine/syncer_unittest.cc (right):

https://codereview.chromium.org/905853002/diff/1/sync/engine/syncer_unittest....
sync/engine/syncer_unittest.cc:2868: TEST_F(SyncerTest,
CommitFailureWithConflict) {
Nice job coming up with this test!

https://codereview.chromium.org/905853002/diff/1/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/905853002/diff/1/sync/protocol/sync.proto#new...
sync/protocol/sync.proto:496: // This flag is set if this GetUpdate request is
due to client receive
couple grammar nits: "receive" -> "receiving", "so client need" -> "so client
needs"

https://codereview.chromium.org/905853002/diff/1/sync/protocol/sync.proto#new...
sync/protocol/sync.proto:500: // Introduced in M41.
This should be m42, 41 has already been cut.

https://codereview.chromium.org/905853002/diff/1/sync/sessions/data_type_trac...
File sync/sessions/data_type_tracker.h (right):

https://codereview.chromium.org/905853002/diff/1/sync/sessions/data_type_trac...
sync/sessions/data_type_tracker.h:81: // returns true if this type is requesting
a sync to resolve conflict issue.
grammar nit: Start sentence with capital ("returns" -> "Returns")

[Gang Wu, 2015-02-06 22:58:19]

updated base on comments.
is it ok to add integration test in another CL?

https://codereview.chromium.org/905853002/diff/1/sync/engine/syncer_unittest.cc
File sync/engine/syncer_unittest.cc (right):

https://codereview.chromium.org/905853002/diff/1/sync/engine/syncer_unittest....
sync/engine/syncer_unittest.cc:2868: TEST_F(SyncerTest,
CommitFailureWithConflict) {
On 2015/02/06 22:37:01, Nicolas Zea wrote:
> Nice job coming up with this test!

Done.

https://codereview.chromium.org/905853002/diff/1/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/905853002/diff/1/sync/protocol/sync.proto#new...
sync/protocol/sync.proto:496: // This flag is set if this GetUpdate request is
due to client receive
On 2015/02/06 22:37:01, Nicolas Zea wrote:
> couple grammar nits: "receive" -> "receiving", "so client need" -> "so client
> needs"

Done.

https://codereview.chromium.org/905853002/diff/1/sync/protocol/sync.proto#new...
sync/protocol/sync.proto:500: // Introduced in M41.
On 2015/02/06 22:37:01, Nicolas Zea wrote:
> This should be m42, 41 has already been cut.

Done.

https://codereview.chromium.org/905853002/diff/1/sync/sessions/data_type_trac...
File sync/sessions/data_type_tracker.h (right):

https://codereview.chromium.org/905853002/diff/1/sync/sessions/data_type_trac...
sync/sessions/data_type_tracker.h:81: // returns true if this type is requesting
a sync to resolve conflict issue.
On 2015/02/06 22:37:01, Nicolas Zea wrote:
> grammar nit: Start sentence with capital ("returns" -> "Returns")

Done.

[Nicolas Zea, 2015-02-09 17:30:31]

Fair enough, this CL is large enough as it is. LGTM

[Gang Wu, 2015-02-09 17:56:39]

The CQ bit was checked by gangwu@chromium.org

[commit-bot: I haz the power, 2015-02-09 17:57:38]

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

[commit-bot: I haz the power, 2015-02-09 18:01:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-09 18:02:32]

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

[Ryan Hamilton, 2015-02-09 18:31:01]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/891123003/ by rch@chromium.org.

The reason for reverting is: Looks like this broke the build

Undefined symbols for architecture armv7:
 
"syncer::sessions::test_util::SimulateDownloadUpdatesFailed(syncer::EnumSet<syncer::ModelType,
(syncer::ModelType)2, (syncer::ModelType)33>, syncer::sessions::NudgeTracker
const&, syncer::sessions::SyncSession*)", referenced from:
     
syncer::BackoffTriggersSyncSchedulerTest_FailDownloadOnceThenSucceed_Test::TestBody()
in sync_scheduler_unittest.o
     
syncer::BackoffTriggersSyncSchedulerTest_FailDownloadTwice_Test::TestBody() in
sync_scheduler_unittest.o
      syncer::SyncSchedulerTest_FailedRetry_Test::TestBody() in
sync_scheduler_unittest.o
.

[Gang Wu, 2015-02-11 20:12:07]

The CQ bit was checked by gangwu@chromium.org

[commit-bot: I haz the power, 2015-02-11 20:13:57]

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

[commit-bot: I haz the power, 2015-02-11 20:44:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-11 20:45:02]

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