summaryrefslogtreecommitdiffstats
path: root/sync
diff options
context:
space:
mode:
authorrlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2012-03-24 02:17:30 +0000
committerrlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2012-03-24 02:17:30 +0000
commitde4963c3f017007f36853564f4ed2ccbf299f454 (patch)
tree9d68f678cfceb2c61d29bc68fca27b421e2194b8 /sync
parentc87efedd255ab42f25fc44588ecdb22e7af43bcb (diff)
downloadchromium_src-de4963c3f017007f36853564f4ed2ccbf299f454.zip
chromium_src-de4963c3f017007f36853564f4ed2ccbf299f454.tar.gz
chromium_src-de4963c3f017007f36853564f4ed2ccbf299f454.tar.bz2
sync: Count and report reflected updates
Many of the updates a sync client receives are echoes of its own changes. This patch attempts to count how often these updates are received by comparing the version of downloaded updates against the local version. These counts are exposed locally through AllStatus/about:sync. We also upload this information to the server through the ClientDebugInfo mechanism. BUG=117565 TEST= Review URL: http://codereview.chromium.org/9702083 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128659 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'sync')
-rw-r--r--sync/engine/verify_updates_command.cc66
-rw-r--r--sync/protocol/client_debug_info.proto7
-rw-r--r--sync/protocol/get_updates_caller_info.proto45
-rw-r--r--sync/protocol/sync.proto35
-rw-r--r--sync/protocol/sync_proto.gyp1
-rw-r--r--sync/sessions/session_state.cc6
-rw-r--r--sync/sessions/session_state.h3
-rw-r--r--sync/sessions/session_state_unittest.cc9
-rw-r--r--sync/sessions/status_controller.cc6
-rw-r--r--sync/sessions/status_controller.h1
-rw-r--r--sync/sessions/sync_session.cc1
11 files changed, 128 insertions, 52 deletions
diff --git a/sync/engine/verify_updates_command.cc b/sync/engine/verify_updates_command.cc
index ab555a2..c4ebe67 100644
--- a/sync/engine/verify_updates_command.cc
+++ b/sync/engine/verify_updates_command.cc
@@ -22,6 +22,54 @@ using syncable::WriteTransaction;
using syncable::GET_BY_ID;
using syncable::SYNCER;
+namespace {
+
+// This function attempts to determine whether or not this update is genuinely
+// new, or if it is a reflection of one of our own commits.
+//
+// There is a known inaccuracy in its implementation. If this update ends up
+// being applied to a local item with a different ID, we will count the change
+// as being a non-reflection update. Fortunately, the server usually updates
+// our IDs correctly in its commit response, so a new ID during GetUpdate should
+// be rare.
+//
+// The only secnarios I can think of where this might happen are:
+// - We commit a new item to the server, but we don't persist the
+// server-returned new ID to the database before we shut down. On the GetUpdate
+// following the next restart, we will receive an update from the server that
+// updates its local ID.
+// - When two attempts to create an item with identical UNIQUE_CLIENT_TAG values
+// collide at the server. I have seen this in testing. When it happens, the
+// test server will send one of the clients a response to upate its local ID so
+// that both clients will refer to the item using the same ID going forward. In
+// this case, we're right to assume that the update is not a reflection.
+//
+// For more information, see SyncerUtil::FindLocalIdToUpdate().
+bool UpdateContainsNewVersion(syncable::BaseTransaction *trans,
+ const SyncEntity &update) {
+ int64 existing_version = -1; // The server always sends positive versions.
+ syncable::Entry existing_entry(trans, GET_BY_ID, update.id());
+ if (existing_entry.good())
+ existing_version = existing_entry.Get(syncable::BASE_VERSION);
+
+ return existing_version < update.version();
+}
+
+// In the event that IDs match, but tags differ AttemptReuniteClient tag
+// will have refused to unify the update.
+// We should not attempt to apply it at all since it violates consistency
+// rules.
+VerifyResult VerifyTagConsistency(const SyncEntity& entry,
+ const syncable::MutableEntry& same_id) {
+ if (entry.has_client_defined_unique_tag() &&
+ entry.client_defined_unique_tag() !=
+ same_id.Get(syncable::UNIQUE_CLIENT_TAG)) {
+ return VERIFY_FAIL;
+ }
+ return VERIFY_UNDECIDED;
+}
+} // namespace
+
VerifyUpdatesCommand::VerifyUpdatesCommand() {}
VerifyUpdatesCommand::~VerifyUpdatesCommand() {}
@@ -62,6 +110,8 @@ SyncerError VerifyUpdatesCommand::ModelChangingExecuteImpl(
session->routing_info());
status->mutable_update_progress()->AddVerifyResult(result.value, update);
status->increment_num_updates_downloaded_by(1);
+ if (!UpdateContainsNewVersion(&trans, update))
+ status->increment_num_reflected_updates_downloaded_by(1);
if (update.deleted())
status->increment_num_tombstone_updates_downloaded_by(1);
}
@@ -69,22 +119,6 @@ SyncerError VerifyUpdatesCommand::ModelChangingExecuteImpl(
return SYNCER_OK;
}
-namespace {
-// In the event that IDs match, but tags differ AttemptReuniteClient tag
-// will have refused to unify the update.
-// We should not attempt to apply it at all since it violates consistency
-// rules.
-VerifyResult VerifyTagConsistency(const SyncEntity& entry,
- const syncable::MutableEntry& same_id) {
- if (entry.has_client_defined_unique_tag() &&
- entry.client_defined_unique_tag() !=
- same_id.Get(syncable::UNIQUE_CLIENT_TAG)) {
- return VERIFY_FAIL;
- }
- return VERIFY_UNDECIDED;
-}
-} // namespace
-
VerifyUpdatesCommand::VerifyUpdateResult VerifyUpdatesCommand::VerifyUpdate(
syncable::WriteTransaction* trans, const SyncEntity& entry,
const ModelSafeRoutingInfo& routes) {
diff --git a/sync/protocol/client_debug_info.proto b/sync/protocol/client_debug_info.proto
index 15216f8..6a0fa54 100644
--- a/sync/protocol/client_debug_info.proto
+++ b/sync/protocol/client_debug_info.proto
@@ -11,6 +11,8 @@ option retain_unknown_fields = true;
package sync_pb;
+import "get_updates_caller_info.proto";
+
// The additional info here is from SyncerStatus. They get sent when the event
// SYNC_CYCLE_COMPLETED is sent.
message SyncCycleCompletedEventInfo {
@@ -31,6 +33,11 @@ message SyncCycleCompletedEventInfo {
optional int32 num_hierarchy_conflicts = 5;
optional int32 num_simple_conflicts = 6;
optional int32 num_server_conflicts = 7;
+
+ // Counts to track the effective usefulness of our GetUpdate requests.
+ optional int32 num_updates_downloaded = 8;
+ optional int32 num_reflected_updates_downloaded = 9;
+ optional GetUpdatesCallerInfo caller_info = 10;
}
message DebugEventInfo {
diff --git a/sync/protocol/get_updates_caller_info.proto b/sync/protocol/get_updates_caller_info.proto
new file mode 100644
index 0000000..41ad2ef
--- /dev/null
+++ b/sync/protocol/get_updates_caller_info.proto
@@ -0,0 +1,45 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+option retain_unknown_fields = true;
+
+package sync_pb;
+
+message GetUpdatesCallerInfo {
+ enum GetUpdatesSource {
+ UNKNOWN = 0; // The source was not set by the caller.
+ FIRST_UPDATE = 1; // First request after browser restart. Not to
+ // be confused with "NEW_CLIENT".
+ LOCAL = 2; // The source of the update was a local change.
+ NOTIFICATION = 3; // The source of the update was a p2p notification.
+ PERIODIC = 4; // The source of the update was periodic polling.
+ SYNC_CYCLE_CONTINUATION = 5; // The source of the update was a
+ // continuation of a previous update.
+ CLEAR_PRIVATE_DATA = 6; // Source is a call to remove all private data
+ NEWLY_SUPPORTED_DATATYPE = 7; // The client is in configuration mode
+ // because it's syncing all datatypes, and
+ // support for a new datatype was recently
+ // released via a software auto-update.
+ MIGRATION = 8; // The client is in configuration mode because a
+ // MIGRATION_DONE error previously returned by the
+ // server necessitated resynchronization.
+ NEW_CLIENT = 9; // The client is in configuration mode because the
+ // user enabled sync for the first time. Not to be
+ // confused with FIRST_UPDATE.
+ RECONFIGURATION = 10; // The client is in configuration mode because the
+ // user opted to sync a different set of datatypes.
+ DATATYPE_REFRESH = 11; // A datatype has requested a refresh. This is
+ // typically used when datatype's have custom
+ // sync UI, e.g. sessions.
+ }
+
+ required GetUpdatesSource source = 1;
+
+ // True only if notifications were enabled for this GetUpdateMessage.
+ optional bool notifications_enabled = 2;
+};
+
diff --git a/sync/protocol/sync.proto b/sync/protocol/sync.proto
index 6191000..dccf99f 100644
--- a/sync/protocol/sync.proto
+++ b/sync/protocol/sync.proto
@@ -19,6 +19,7 @@ import "app_setting_specifics.proto";
import "app_specifics.proto";
import "autofill_specifics.proto";
import "bookmark_specifics.proto";
+import "get_updates_caller_info.proto";
import "extension_setting_specifics.proto";
import "extension_specifics.proto";
import "nigori_specifics.proto";
@@ -355,40 +356,6 @@ message CommitMessage {
repeated ChromiumExtensionsActivity extensions_activity = 3;
};
-message GetUpdatesCallerInfo {
- enum GetUpdatesSource {
- UNKNOWN = 0; // The source was not set by the caller.
- FIRST_UPDATE = 1; // First request after browser restart. Not to
- // be confused with "NEW_CLIENT".
- LOCAL = 2; // The source of the update was a local change.
- NOTIFICATION = 3; // The source of the update was a p2p notification.
- PERIODIC = 4; // The source of the update was periodic polling.
- SYNC_CYCLE_CONTINUATION = 5; // The source of the update was a
- // continuation of a previous update.
- CLEAR_PRIVATE_DATA = 6; // Source is a call to remove all private data
- NEWLY_SUPPORTED_DATATYPE = 7; // The client is in configuration mode
- // because it's syncing all datatypes, and
- // support for a new datatype was recently
- // released via a software auto-update.
- MIGRATION = 8; // The client is in configuration mode because a
- // MIGRATION_DONE error previously returned by the
- // server necessitated resynchronization.
- NEW_CLIENT = 9; // The client is in configuration mode because the
- // user enabled sync for the first time. Not to be
- // confused with FIRST_UPDATE.
- RECONFIGURATION = 10; // The client is in configuration mode because the
- // user opted to sync a different set of datatypes.
- DATATYPE_REFRESH = 11; // A datatype has requested a refresh. This is
- // typically used when datatype's have custom
- // sync UI, e.g. sessions.
- }
-
- required GetUpdatesSource source = 1;
-
- // True only if notifications were enabled for this GetUpdateMessage.
- optional bool notifications_enabled = 2;
-};
-
message DataTypeProgressMarker {
// An integer identifying the data type whose progress is tracked by this
// marker. The legitimate values of this field correspond to the protobuf
diff --git a/sync/protocol/sync_proto.gyp b/sync/protocol/sync_proto.gyp
index 28198b8..c9d5627 100644
--- a/sync/protocol/sync_proto.gyp
+++ b/sync/protocol/sync_proto.gyp
@@ -18,6 +18,7 @@
'bookmark_specifics.proto',
'client_commands.proto',
'client_debug_info.proto',
+ 'get_updates_caller_info.proto',
'encryption.proto',
'extension_setting_specifics.proto',
'extension_specifics.proto',
diff --git a/sync/sessions/session_state.cc b/sync/sessions/session_state.cc
index 2c68125..f49c41d 100644
--- a/sync/sessions/session_state.cc
+++ b/sync/sessions/session_state.cc
@@ -50,6 +50,7 @@ SyncerStatus::SyncerStatus()
num_successful_bookmark_commits(0),
num_updates_downloaded_total(0),
num_tombstone_updates_downloaded_total(0),
+ num_reflected_updates_downloaded_total(0),
num_local_overwrites(0),
num_server_overwrites(0) {
}
@@ -67,6 +68,8 @@ DictionaryValue* SyncerStatus::ToValue() const {
num_updates_downloaded_total);
value->SetInteger("numTombstoneUpdatesDownloadedTotal",
num_tombstone_updates_downloaded_total);
+ value->SetInteger("numReflectedUpdatesDownloadedTotal",
+ num_reflected_updates_downloaded_total);
value->SetInteger("numLocalOverwrites", num_local_overwrites);
value->SetInteger("numServerOverwrites", num_server_overwrites);
return value;
@@ -113,6 +116,7 @@ SyncSessionSnapshot::SyncSessionSnapshot(
int num_server_conflicts,
bool did_commit_items,
const SyncSourceInfo& source,
+ bool notifications_enabled,
size_t num_entries,
base::Time sync_start_time,
bool retry_scheduled)
@@ -131,6 +135,7 @@ SyncSessionSnapshot::SyncSessionSnapshot(
num_server_conflicts(num_server_conflicts),
did_commit_items(did_commit_items),
source(source),
+ notifications_enabled(notifications_enabled),
num_entries(num_entries),
sync_start_time(sync_start_time),
retry_scheduled(retry_scheduled) {
@@ -170,6 +175,7 @@ DictionaryValue* SyncSessionSnapshot::ToValue() const {
value->SetBoolean("didCommitItems", did_commit_items);
value->SetInteger("numEntries", num_entries);
value->Set("source", source.ToValue());
+ value->SetBoolean("notificationsEnabled", notifications_enabled);
return value;
}
diff --git a/sync/sessions/session_state.h b/sync/sessions/session_state.h
index 676fe5b..7403758 100644
--- a/sync/sessions/session_state.h
+++ b/sync/sessions/session_state.h
@@ -72,6 +72,7 @@ struct SyncerStatus {
// Download event counters.
int num_updates_downloaded_total;
int num_tombstone_updates_downloaded_total;
+ int num_reflected_updates_downloaded_total;
// If the syncer encountered a MIGRATION_DONE code, these are the types that
// the client must now "migrate", by purging and re-downloading all updates.
@@ -121,6 +122,7 @@ struct SyncSessionSnapshot {
int num_server_conflicts,
bool did_commit_items,
const SyncSourceInfo& source,
+ bool notifications_enabled,
size_t num_entries,
base::Time sync_start_time,
bool retry_scheduled);
@@ -146,6 +148,7 @@ struct SyncSessionSnapshot {
const int num_server_conflicts;
const bool did_commit_items;
const SyncSourceInfo source;
+ const bool notifications_enabled;
const size_t num_entries;
base::Time sync_start_time;
const bool retry_scheduled;
diff --git a/sync/sessions/session_state_unittest.cc b/sync/sessions/session_state_unittest.cc
index b18964a..a37dc17 100644
--- a/sync/sessions/session_state_unittest.cc
+++ b/sync/sessions/session_state_unittest.cc
@@ -49,11 +49,12 @@ TEST_F(SessionStateTest, SyncerStatusToValue) {
status.num_successful_bookmark_commits = 10;
status.num_updates_downloaded_total = 100;
status.num_tombstone_updates_downloaded_total = 200;
+ status.num_reflected_updates_downloaded_total = 50;
status.num_local_overwrites = 15;
status.num_server_overwrites = 18;
scoped_ptr<DictionaryValue> value(status.ToValue());
- EXPECT_EQ(7u, value->size());
+ EXPECT_EQ(8u, value->size());
ExpectDictBooleanValue(status.invalid_store, *value, "invalidStore");
ExpectDictIntegerValue(status.num_successful_commits,
*value, "numSuccessfulCommits");
@@ -63,6 +64,8 @@ TEST_F(SessionStateTest, SyncerStatusToValue) {
*value, "numUpdatesDownloadedTotal");
ExpectDictIntegerValue(status.num_tombstone_updates_downloaded_total,
*value, "numTombstoneUpdatesDownloadedTotal");
+ ExpectDictIntegerValue(status.num_reflected_updates_downloaded_total,
+ *value, "numReflectedUpdatesDownloadedTotal");
ExpectDictIntegerValue(status.num_local_overwrites,
*value, "numLocalOverwrites");
ExpectDictIntegerValue(status.num_server_overwrites,
@@ -141,11 +144,12 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
kNumServerConflicts,
kDidCommitItems,
source,
+ false,
0,
base::Time::Now(),
false);
scoped_ptr<DictionaryValue> value(snapshot.ToValue());
- EXPECT_EQ(15u, value->size());
+ EXPECT_EQ(16u, value->size());
ExpectDictDictionaryValue(*expected_syncer_status_value, *value,
"syncerStatus");
ExpectDictIntegerValue(kNumServerChangesRemaining, *value,
@@ -169,6 +173,7 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
ExpectDictBooleanValue(kDidCommitItems, *value,
"didCommitItems");
ExpectDictDictionaryValue(*expected_source_value, *value, "source");
+ ExpectDictBooleanValue(false, *value, "notificationsEnabled");
}
} // namespace
diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc
index 2ae1a96..19fdbaf 100644
--- a/sync/sessions/status_controller.cc
+++ b/sync/sessions/status_controller.cc
@@ -116,6 +116,12 @@ void StatusController::increment_num_tombstone_updates_downloaded_by(
value;
}
+void StatusController::increment_num_reflected_updates_downloaded_by(
+ int value) {
+ shared_.syncer_status.mutate()->num_reflected_updates_downloaded_total +=
+ value;
+}
+
void StatusController::set_num_server_changes_remaining(
int64 changes_remaining) {
if (shared_.num_server_changes_remaining.value() != changes_remaining)
diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h
index 6a2491c..c2a3cfc 100644
--- a/sync/sessions/status_controller.h
+++ b/sync/sessions/status_controller.h
@@ -214,6 +214,7 @@ class StatusController {
void increment_num_successful_bookmark_commits();
void increment_num_updates_downloaded_by(int value);
void increment_num_tombstone_updates_downloaded_by(int value);
+ void increment_num_reflected_updates_downloaded_by(int value);
void set_types_needing_local_migration(syncable::ModelTypeSet types);
void set_unsynced_handles(const std::vector<int64>& unsynced_handles);
void increment_num_local_overwrites();
diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index 2a93b44..307ae97 100644
--- a/sync/sessions/sync_session.cc
+++ b/sync/sessions/sync_session.cc
@@ -165,6 +165,7 @@ SyncSessionSnapshot SyncSession::TakeSnapshot() const {
status_controller_->TotalNumServerConflictingItems(),
status_controller_->did_commit_items(),
source_,
+ context_->notifications_enabled(),
dir->GetEntriesCount(),
status_controller_->sync_start_time(),
!Succeeded());