sync: Count and report reflected updates

sync/engine/verify_updates_command.cc

 // 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.
 
 #include "sync/engine/verify_updates_command.h"
 
 #include <string>
 
 #include "base/location.h"
 #include "sync/engine/syncer.h"
 #include "sync/engine/syncer_proto_util.h"
 #include "sync/engine/syncer_types.h"
 #include "sync/engine/syncer_util.h"
 #include "sync/engine/syncproto.h"
 #include "sync/protocol/bookmark_specifics.pb.h"
 #include "sync/syncable/syncable.h"
 
 namespace browser_sync {
 
 using syncable::WriteTransaction;
 
 using syncable::GET_BY_ID;
 using syncable::SYNCER;
 
+namespace {
+
+// Returns true if the update's version is greather than the version of the

[tim (not reviewing), 2012/03/20 20:43:53]

nit - greater

[rlarocque, 2012/03/21 17:42:34]

Done.

+// existing matching entry.  Will return true if there is no matching entry.
+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());

[tim (not reviewing), 2012/03/20 20:43:53]

When we create an item (version 0), we expect to see that item come back down
with updated progress markers and the like at some point.  Does this count those
as reflections?

SyncerUtil::FindLocalIdToUpdate has some interesting cases, particularly
surrounding lost commit responses. For instance, super rarely we could get an
update for a logically equivalent item but with a different id (actually two
cases, one is we lost a commit response and we still have the client id locally,
and the other case is a race).  Depending on the answer to the question above
about expected-echos, I think what you have actually does what you want... but
double checking. I'm still wrapping my head around the explicit goal... may
warrant a comment.

[rlarocque, 2012/03/21 17:42:34]

Interesting.  I hadn't fully considered the new ID case.  

I believe the goal is to count updates that do not improve the user's
experience.  Receiving bookmarks created on other clients makes users happy. 
Receiving redundant copies of their own bookmarks does not.  

By that logic, we should count these server-provided-ID updates as reflections
if they don't include data that came form other clients.  I'm not sure how we
could implement it, though.  Even if we did associate the new server-provided ID
with the existing local ID, we wouldn't be allowed to compare versions between
the two.  

What could we use to differentiate between an update with a new ID and a
reflection of our own data, and one that includes a new ID with data that has
been modified by other clients?  I'm not sure there is such a thing.  

The number of these updates should be very small compared to the number of
"normal" updates.  It wouldn't bother me too much to leave this inaccuracy as
is.  It would deserve some commenting, though.

Another alternative would be to have a separate counter for this ambiguous case.
 That would be more precise, but I worry that it would be too confusing. 
Thoughts?

[tim (not reviewing), 2012/03/21 18:23:57]

Makes sense.
I want to say this sounds like FindLocalIdToUpdate, and you could use the
originator_cache_guid for this.  But I fear I may be missing something.
Agree, I'm not overly concerned about the precision at that level, but it's good
to be able to explain what we're counting.  For now at least, my gut tells me
not to bother splitting it out into two counters.

+  if (existing_entry.good()) {

[tim (not reviewing), 2012/03/20 20:43:53]

nit - no braces around single line ifs in this file.

[rlarocque, 2012/03/21 17:42:34]

Done.

+    existing_version = existing_entry.Get(syncable::BASE_VERSION);
+  }
+
+  return existing_version < update.version();

[tim (not reviewing), 2012/03/20 20:43:53]

Does this catch redeliveries in general, or only of changes from this client? 
Would it make sense to check originator_cache_guid?

[rlarocque, 2012/03/21 17:42:34]

I assumed that we're looking for redeliveries in general, so we don't need to
check the originator.  

Raz, which count would you prefer?

+}
+
+// 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() {}
 
 std::set<ModelSafeGroup> VerifyUpdatesCommand::GetGroupsToChange(
     const sessions::SyncSession& session) const {
   std::set<ModelSafeGroup> groups_with_updates;
 
   const GetUpdatesResponse& updates =
       session.status_controller().updates_response().get_updates();
   for (int i = 0; i < updates.entries().size(); i++) {
@@ skipping 20 matching lines @@
         *reinterpret_cast<const SyncEntity *>(&(updates.entries(i)));
     ModelSafeGroup g = GetGroupForModelType(update.GetModelType(),
                                             session->routing_info());
     if (g != status->group_restriction())
       continue;
 
     VerifyUpdateResult result = VerifyUpdate(&trans, update,
                                              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_echo_updates_downloaded_by(1);
     if (update.deleted())
       status->increment_num_tombstone_updates_downloaded_by(1);
   }
 
   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) {
   syncable::Id id = entry.id();
   VerifyUpdateResult result = {VERIFY_FAIL, GROUP_PASSIVE};
 
   const bool deleted = entry.has_deleted() && entry.deleted();
   const bool is_directory = entry.IsFolder();
   const syncable::ModelType model_type = entry.GetModelType();
 
@@ skipping 32 matching lines @@
         deleted, is_directory, model_type);
   }
 
   if (VERIFY_UNDECIDED == result.value)
     result.value = VERIFY_SUCCESS;  // No news is good news.
 
   return result;  // This might be VERIFY_SUCCESS as well
 }
 
 }  // namespace browser_sync