diff options
author | rch <rch@chromium.org> | 2015-02-09 10:32:54 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-02-09 18:33:20 +0000 |
commit | 796a0ab344091f0782693b636206286e4784a078 (patch) | |
tree | 0dd9a9b1c1314267846b3f9f7b10a70bcaa44941 | |
parent | bd7a27361b8a72157ddbf25733a521022641481c (diff) | |
download | chromium_src-796a0ab344091f0782693b636206286e4784a078.zip chromium_src-796a0ab344091f0782693b636206286e4784a078.tar.gz chromium_src-796a0ab344091f0782693b636206286e4784a078.tar.bz2 |
Revert of Sync commit errors should temporarily re-enable trigger pre-commit getupdates (patchset #2 id:20001 of https://codereview.chromium.org/905853002/)
Reason for revert:
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
Original issue's description:
> 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}
TBR=zea@chromium.org,stanisc@chromium.org,maniscalco@chromium.org,pvalenzuela@chromium.org,gangwu@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=324893
Review URL: https://codereview.chromium.org/891123003
Cr-Commit-Position: refs/heads/master@{#315346}
-rw-r--r-- | sync/engine/commit.cc | 4 | ||||
-rw-r--r-- | sync/engine/commit.h | 9 | ||||
-rw-r--r-- | sync/engine/sync_scheduler_impl.cc | 4 | ||||
-rw-r--r-- | sync/engine/sync_scheduler_unittest.cc | 7 | ||||
-rw-r--r-- | sync/engine/syncer.cc | 18 | ||||
-rw-r--r-- | sync/engine/syncer.h | 10 | ||||
-rw-r--r-- | sync/engine/syncer_unittest.cc | 29 | ||||
-rw-r--r-- | sync/protocol/sync.proto | 7 | ||||
-rw-r--r-- | sync/sessions/data_type_tracker.cc | 16 | ||||
-rw-r--r-- | sync/sessions/data_type_tracker.h | 10 | ||||
-rw-r--r-- | sync/sessions/nudge_tracker.cc | 6 | ||||
-rw-r--r-- | sync/sessions/nudge_tracker.h | 4 | ||||
-rw-r--r-- | sync/sessions/nudge_tracker_unittest.cc | 6 | ||||
-rw-r--r-- | sync/sessions/test_util.cc | 20 | ||||
-rw-r--r-- | sync/sessions/test_util.h | 10 |
15 files changed, 49 insertions, 111 deletions
diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc index 0561e9b..509e08d 100644 --- a/sync/engine/commit.cc +++ b/sync/engine/commit.cc @@ -83,7 +83,6 @@ Commit* Commit::Init( } SyncerError Commit::PostAndProcessResponse( - sessions::NudgeTracker* nudge_tracker, sessions::SyncSession* session, sessions::StatusController* status, ExtensionsActivity* extensions_activity) { @@ -154,9 +153,6 @@ SyncerError Commit::PostAndProcessResponse( "type", ModelTypeToString(it->first)); SyncerError type_result = it->second->ProcessCommitResponse(response_, status); - if (type_result == SERVER_RETURN_CONFLICT) { - nudge_tracker->RecordCommitConflict(it->first); - } if (processing_result == SYNCER_OK && type_result != SYNCER_OK) { processing_result = type_result; } diff --git a/sync/engine/commit.h b/sync/engine/commit.h index ee81577..993bd872 100644 --- a/sync/engine/commit.h +++ b/sync/engine/commit.h @@ -14,7 +14,6 @@ #include "sync/internal_api/public/engine/model_safe_worker.h" #include "sync/internal_api/public/util/syncer_error.h" #include "sync/protocol/sync.pb.h" -#include "sync/sessions/nudge_tracker.h" #include "sync/util/extensions_activity.h" namespace syncer { @@ -55,10 +54,10 @@ class SYNC_EXPORT_PRIVATE Commit { CommitProcessor* commit_processor, ExtensionsActivity* extensions_activity); - SyncerError PostAndProcessResponse(sessions::NudgeTracker* nudge_tracker, - sessions::SyncSession* session, - sessions::StatusController* status, - ExtensionsActivity* extensions_activity); + SyncerError PostAndProcessResponse( + sessions::SyncSession* session, + sessions::StatusController* status, + ExtensionsActivity* extensions_activity); // Cleans up state associated with this commit. Must be called before the // destructor. diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc index 6f76be0..d279fd0 100644 --- a/sync/engine/sync_scheduler_impl.cc +++ b/sync/engine/sync_scheduler_impl.cc @@ -465,7 +465,9 @@ void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) { << ModelTypeSetToString(session_context_->GetEnabledTypes()); scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this)); bool premature_exit = !syncer_->NormalSyncShare( - GetEnabledAndUnthrottledTypes(), &nudge_tracker_, session.get()); + GetEnabledAndUnthrottledTypes(), + nudge_tracker_, + session.get()); AdjustPolling(FORCE_RESET); // Don't run poll job till the next time poll timer fires. do_poll_after_credentials_updated_ = false; diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc index 90b20a0..21d7f28 100644 --- a/sync/engine/sync_scheduler_unittest.cc +++ b/sync/engine/sync_scheduler_unittest.cc @@ -44,10 +44,9 @@ using sync_pb::GetUpdatesCallerInfo; class MockSyncer : public Syncer { public: MockSyncer(); - MOCK_METHOD3(NormalSyncShare, - bool(ModelTypeSet, - sessions::NudgeTracker*, - sessions::SyncSession*)); + MOCK_METHOD3(NormalSyncShare, bool(ModelTypeSet, + const sessions::NudgeTracker&, + sessions::SyncSession*)); MOCK_METHOD3(ConfigureSyncShare, bool(ModelTypeSet, sync_pb::GetUpdatesCallerInfo::GetUpdatesSource, diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc index ef759b8..bae809e 100644 --- a/sync/engine/syncer.cc +++ b/sync/engine/syncer.cc @@ -55,13 +55,13 @@ bool Syncer::ExitRequested() { } bool Syncer::NormalSyncShare(ModelTypeSet request_types, - NudgeTracker* nudge_tracker, + const NudgeTracker& nudge_tracker, SyncSession* session) { HandleCycleBegin(session); - if (nudge_tracker->IsGetUpdatesRequired() || + if (nudge_tracker.IsGetUpdatesRequired() || session->context()->ShouldFetchUpdatesBeforeCommit()) { VLOG(1) << "Downloading types " << ModelTypeSetToString(request_types); - NormalGetUpdatesDelegate normal_delegate(*nudge_tracker); + NormalGetUpdatesDelegate normal_delegate(nudge_tracker); GetUpdatesProcessor get_updates_processor( session->context()->model_type_registry()->update_handler_map(), normal_delegate); @@ -70,18 +70,18 @@ bool Syncer::NormalSyncShare(ModelTypeSet request_types, session, &get_updates_processor, kCreateMobileBookmarksFolder)) { - return HandleCycleEnd(session, nudge_tracker->GetLegacySource()); + return HandleCycleEnd(session, nudge_tracker.GetLegacySource()); } } VLOG(1) << "Committing from types " << ModelTypeSetToString(request_types); CommitProcessor commit_processor( session->context()->model_type_registry()->commit_contributor_map()); - SyncerError commit_result = BuildAndPostCommits(request_types, nudge_tracker, - session, &commit_processor); + SyncerError commit_result = + BuildAndPostCommits(request_types, session, &commit_processor); session->mutable_status_controller()->set_commit_result(commit_result); - return HandleCycleEnd(session, nudge_tracker->GetLegacySource()); + return HandleCycleEnd(session, nudge_tracker.GetLegacySource()); } bool Syncer::ConfigureSyncShare( @@ -160,7 +160,6 @@ bool Syncer::DownloadAndApplyUpdates( } SyncerError Syncer::BuildAndPostCommits(ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, sessions::SyncSession* session, CommitProcessor* commit_processor) { // The ExitRequested() check is unnecessary, since we should start getting @@ -181,7 +180,8 @@ SyncerError Syncer::BuildAndPostCommits(ModelTypeSet requested_types, } SyncerError error = commit->PostAndProcessResponse( - nudge_tracker, session, session->mutable_status_controller(), + session, + session->mutable_status_controller(), session->context()->extensions_activity()); commit->CleanUp(); if (error != SYNCER_OK) { diff --git a/sync/engine/syncer.h b/sync/engine/syncer.h index ca93b3608..02e66e3 100644 --- a/sync/engine/syncer.h +++ b/sync/engine/syncer.h @@ -48,7 +48,7 @@ class SYNC_EXPORT_PRIVATE Syncer { // sync. The |nudge_tracker| contains state that describes why the client is // out of sync and what must be done to bring it back into sync. virtual bool NormalSyncShare(ModelTypeSet request_types, - sessions::NudgeTracker* nudge_tracker, + const sessions::NudgeTracker& nudge_tracker, sessions::SyncSession* session); // Performs an initial download for the |request_types|. It is assumed that @@ -79,10 +79,10 @@ class SYNC_EXPORT_PRIVATE Syncer { // number of unsynced and ready to commit items reaches zero or an error is // encountered. A request to exit early will be treated as an error and will // abort any blocking operations. - SyncerError BuildAndPostCommits(ModelTypeSet request_types, - sessions::NudgeTracker* nudge_tracker, - sessions::SyncSession* session, - CommitProcessor* commit_processor); + SyncerError BuildAndPostCommits( + ModelTypeSet request_types, + sessions::SyncSession* session, + CommitProcessor* commit_processor); void HandleCycleBegin(sessions::SyncSession* session); bool HandleCycleEnd( diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index 5f049ec..471df65 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -262,8 +262,11 @@ class SyncerTest : public testing::Test, // Pretend we've seen a local change, to make the nudge_tracker look normal. nudge_tracker_.RecordLocalChange(ModelTypeSet(BOOKMARKS)); - EXPECT_TRUE(syncer_->NormalSyncShare(context_->GetEnabledTypes(), - &nudge_tracker_, session_.get())); + EXPECT_TRUE( + syncer_->NormalSyncShare( + context_->GetEnabledTypes(), + nudge_tracker_, + session_.get())); } void SyncShareConfigure() { @@ -639,7 +642,8 @@ TEST_F(SyncerTest, GetCommitIdsFiltersThrottledEntries) { ResetSession(); syncer_->NormalSyncShare( Difference(context_->GetEnabledTypes(), ModelTypeSet(BOOKMARKS)), - &nudge_tracker_, session_.get()); + nudge_tracker_, + session_.get()); { // Nothing should have been committed as bookmarks is throttled. @@ -2863,25 +2867,6 @@ TEST_F(SyncerTest, SendDebugInfoEventsOnGetUpdates_PostFailsDontDrop) { EXPECT_EQ(0, mock_server_->last_request().debug_info().events_size()); } -// Tests that commit failure with conflict will trigger GetUpdates for next -// sycle of sync -TEST_F(SyncerTest, CommitFailureWithConflict) { - ConfigureNoGetUpdatesRequired(); - CreateUnsyncedDirectory("X", "id_X"); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); - - SyncShareNudge(); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); - - CreateUnsyncedDirectory("Y", "id_Y"); - mock_server_->set_conflict_n_commits(1); - SyncShareNudge(); - EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired()); - - nudge_tracker_.RecordSuccessfulSyncCycle(); - EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired()); -} - // Tests that sending debug info events on Commit works. TEST_F(SyncerTest, SendDebugInfoEventsOnCommit_HappyCase) { // Make sure GetUpdate isn't call as it would "steal" debug info events before diff --git a/sync/protocol/sync.proto b/sync/protocol/sync.proto index 2c79c22..df302e3 100644 --- a/sync/protocol/sync.proto +++ b/sync/protocol/sync.proto @@ -492,13 +492,6 @@ message GetUpdateTriggers { // // Introduced in M38. optional bool initial_sync_in_progress = 7; - - // This flag is set if this GetUpdate request is due to client receiving - // conflict response from server, so client needs to sync and then resolve - // conflict locally, and then commit again. - // - // Introduced in M42. - optional bool sync_for_resolve_conflict_in_progress = 8; } message GarbageCollectionDirective { diff --git a/sync/sessions/data_type_tracker.cc b/sync/sessions/data_type_tracker.cc index 8103c00..da50bbf 100644 --- a/sync/sessions/data_type_tracker.cc +++ b/sync/sessions/data_type_tracker.cc @@ -15,8 +15,7 @@ DataTypeTracker::DataTypeTracker() : local_nudge_count_(0), local_refresh_request_count_(0), payload_buffer_size_(NudgeTracker::kDefaultMaxPayloadsPerType), - initial_sync_required_(false), - sync_required_to_resolve_conflict_(false) { + initial_sync_required_(false) { } DataTypeTracker::~DataTypeTracker() { } @@ -93,10 +92,6 @@ void DataTypeTracker::RecordInitialSyncRequired() { initial_sync_required_ = true; } -void DataTypeTracker::RecordCommitConflict() { - sync_required_to_resolve_conflict_ = true; -} - void DataTypeTracker::RecordSuccessfulSyncCycle() { // If we were throttled, then we would have been excluded from this cycle's // GetUpdates and Commit actions. Our state remains unchanged. @@ -124,7 +119,6 @@ void DataTypeTracker::RecordSuccessfulSyncCycle() { } initial_sync_required_ = false; - sync_required_to_resolve_conflict_ = false; } // This limit will take effect on all future invalidations received. @@ -139,7 +133,7 @@ bool DataTypeTracker::IsSyncRequired() const { bool DataTypeTracker::IsGetUpdatesRequired() const { return !IsThrottled() && (HasRefreshRequestPending() || HasPendingInvalidation() || - IsInitialSyncRequired() || IsSyncRequiredToResolveConflict()); + IsInitialSyncRequired()); } bool DataTypeTracker::HasLocalChangePending() const { @@ -158,10 +152,6 @@ bool DataTypeTracker::IsInitialSyncRequired() const { return initial_sync_required_; } -bool DataTypeTracker::IsSyncRequiredToResolveConflict() const { - return sync_required_to_resolve_conflict_; -} - void DataTypeTracker::SetLegacyNotificationHint( sync_pb::DataTypeProgressMarker* progress) const { DCHECK(!IsThrottled()) @@ -202,8 +192,6 @@ void DataTypeTracker::FillGetUpdatesTriggersMessage( msg->set_local_modification_nudges(local_nudge_count_); msg->set_datatype_refresh_nudges(local_refresh_request_count_); msg->set_initial_sync_in_progress(initial_sync_required_); - msg->set_sync_for_resolve_conflict_in_progress( - sync_required_to_resolve_conflict_); } bool DataTypeTracker::IsThrottled() const { diff --git a/sync/sessions/data_type_tracker.h b/sync/sessions/data_type_tracker.h index 0842197..c53f393 100644 --- a/sync/sessions/data_type_tracker.h +++ b/sync/sessions/data_type_tracker.h @@ -43,10 +43,6 @@ class DataTypeTracker { // Takes note that initial sync is pending for this type. void RecordInitialSyncRequired(); - // Takes note that the conflict happended for this type, need to sync to - // resolve conflict locally. - void RecordCommitConflict(); - // Records that a sync cycle has been performed successfully. // Generally, this means that all local changes have been committed and all // remote changes have been downloaded, so we can clear any flags related to @@ -78,9 +74,6 @@ class DataTypeTracker { // Returns true if this type is requesting an initial sync. bool IsInitialSyncRequired() const; - // Returns true if this type is requesting a sync to resolve conflict issue. - bool IsSyncRequiredToResolveConflict() const; - // Fills in the legacy invalidaiton payload information fields. void SetLegacyNotificationHint( sync_pb::DataTypeProgressMarker* progress) const; @@ -129,9 +122,6 @@ class DataTypeTracker { // sync. bool initial_sync_required_; - // Set to true if this type need to get update to resolve conflict issue. - bool sync_required_to_resolve_conflict_; - // If !unthrottle_time_.is_null(), this type is throttled and may not download // or commit data until the specified time. base::TimeTicks unthrottle_time_; diff --git a/sync/sessions/nudge_tracker.cc b/sync/sessions/nudge_tracker.cc index 6af6993..9c62b07 100644 --- a/sync/sessions/nudge_tracker.cc +++ b/sync/sessions/nudge_tracker.cc @@ -168,12 +168,6 @@ void NudgeTracker::RecordInitialSyncRequired(syncer::ModelType type) { tracker_it->second->RecordInitialSyncRequired(); } -void NudgeTracker::RecordCommitConflict(syncer::ModelType type) { - TypeTrackerMap::iterator tracker_it = type_trackers_.find(type); - DCHECK(tracker_it != type_trackers_.end()); - tracker_it->second->RecordCommitConflict(); -} - void NudgeTracker::OnInvalidationsEnabled() { invalidations_enabled_ = true; } diff --git a/sync/sessions/nudge_tracker.h b/sync/sessions/nudge_tracker.h index ec2277c..2bf9da1 100644 --- a/sync/sessions/nudge_tracker.h +++ b/sync/sessions/nudge_tracker.h @@ -70,10 +70,6 @@ class SYNC_EXPORT_PRIVATE NudgeTracker { // Take note that an initial sync is pending for this type. void RecordInitialSyncRequired(syncer::ModelType type); - // Takes note that the conflict happended for this type, need to sync to - // resolve conflict locally. - void RecordCommitConflict(syncer::ModelType type); - // These functions should be called to keep this class informed of the status // of the connection to the invalidations server. void OnInvalidationsEnabled(); diff --git a/sync/sessions/nudge_tracker_unittest.cc b/sync/sessions/nudge_tracker_unittest.cc index db2bc3e..c6a9dfb8 100644 --- a/sync/sessions/nudge_tracker_unittest.cc +++ b/sync/sessions/nudge_tracker_unittest.cc @@ -369,12 +369,6 @@ TEST_F(NudgeTrackerTest, IsSyncRequired) { nudge_tracker_.RecordSuccessfulSyncCycle(); EXPECT_FALSE(nudge_tracker_.IsSyncRequired()); - // Sync request for resolve conflict. - nudge_tracker_.RecordCommitConflict(BOOKMARKS); - EXPECT_TRUE(nudge_tracker_.IsSyncRequired()); - nudge_tracker_.RecordSuccessfulSyncCycle(); - EXPECT_FALSE(nudge_tracker_.IsSyncRequired()); - // Local changes. nudge_tracker_.RecordLocalChange(ModelTypeSet(SESSIONS)); EXPECT_TRUE(nudge_tracker_.IsSyncRequired()); diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc index c226a12..1163ea7 100644 --- a/sync/sessions/test_util.cc +++ b/sync/sessions/test_util.cc @@ -46,22 +46,23 @@ void SimulateConfigureConnectionFailure( } void SimulateNormalSuccess(ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, + const sessions::NudgeTracker& nudge_tracker, sessions::SyncSession* session) { session->mutable_status_controller()->set_commit_result(SYNCER_OK); session->mutable_status_controller()->set_last_download_updates_result( SYNCER_OK); } -void SimulateDownloadUpdatesFailed(ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, - sessions::SyncSession* session) { +void SimulateDownloadUpdatesFailed( + ModelTypeSet requested_types, + const sessions::NudgeTracker& nudge_tracker, + sessions::SyncSession* session) { session->mutable_status_controller()->set_last_download_updates_result( SERVER_RETURN_TRANSIENT_ERROR); } void SimulateCommitFailed(ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, + const sessions::NudgeTracker& nudge_tracker, sessions::SyncSession* session) { session->mutable_status_controller()->set_last_get_key_result(SYNCER_OK); session->mutable_status_controller()->set_last_download_updates_result( @@ -70,9 +71,10 @@ void SimulateCommitFailed(ModelTypeSet requested_types, SERVER_RETURN_TRANSIENT_ERROR); } -void SimulateConnectionFailure(ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, - sessions::SyncSession* session) { +void SimulateConnectionFailure( + ModelTypeSet requested_types, + const sessions::NudgeTracker& nudge_tracker, + sessions::SyncSession* session) { session->mutable_status_controller()->set_last_download_updates_result( NETWORK_CONNECTION_UNAVAILABLE); } @@ -116,7 +118,7 @@ void SimulatePollIntervalUpdateImpl( void SimulateSessionsCommitDelayUpdateImpl( ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, + const sessions::NudgeTracker& nudge_tracker, sessions::SyncSession* session, const base::TimeDelta& new_delay) { SimulateNormalSuccess(requested_types, nudge_tracker, session); diff --git a/sync/sessions/test_util.h b/sync/sessions/test_util.h index 3e905dd..357637e 100644 --- a/sync/sessions/test_util.h +++ b/sync/sessions/test_util.h @@ -35,16 +35,16 @@ void SimulateConfigureConnectionFailure( // Normal mode sync cycle successes and failures. void SimulateNormalSuccess(ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, + const sessions::NudgeTracker& nudge_tracker, sessions::SyncSession* session); void SimulateDownloadUpdatesFailed(ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, + const sessions::NudgeTracker& nudge_tracker, sessions::SyncSession* session); void SimulateCommitFailed(ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, + const sessions::NudgeTracker& nudge_tracker, sessions::SyncSession* session); void SimulateConnectionFailure(ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, + const sessions::NudgeTracker& nudge_tracker, sessions::SyncSession* session); // Poll successes and failures. @@ -73,7 +73,7 @@ void SimulatePollIntervalUpdateImpl( // Works with normal cycles. void SimulateSessionsCommitDelayUpdateImpl( ModelTypeSet requested_types, - sessions::NudgeTracker* nudge_tracker, + const sessions::NudgeTracker& nudge_tracker, sessions::SyncSession* session, const base::TimeDelta& new_delay); |