[Sync] Start implementation of migration for USS.

components/sync/engine_impl/model_type_registry.cc

Index: components/sync/engine_impl/model_type_registry.cc
diff --git a/components/sync/engine_impl/model_type_registry.cc b/components/sync/engine_impl/model_type_registry.cc
index 9cabe008079c3893c332060342ff7204916ae41a..597517c787ef28d25ba427d656e12d67e69c254b 100644
--- a/components/sync/engine_impl/model_type_registry.cc
+++ b/components/sync/engine_impl/model_type_registry.cc
@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram.h"
 #include "base/observer_list.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/sync/base/cryptographer.h"
@@ -20,6 +21,7 @@
 #include "components/sync/engine_impl/directory_commit_contributor.h"
 #include "components/sync/engine_impl/directory_update_handler.h"
 #include "components/sync/engine_impl/model_type_worker.h"
+#include "components/sync/engine_impl/uss_migrator.h"
 
 namespace syncer {
 
@@ -54,9 +56,9 @@ void CommitQueueProxy::EnqueueForCommit(const CommitRequestDataList& list) {
 
 ModelTypeRegistry::ModelTypeRegistry(
     const std::vector<scoped_refptr<ModelSafeWorker>>& workers,
-    syncable::Directory* directory,
+    UserShare* user_share,
     NudgeHandler* nudge_handler)
-    : directory_(directory),
+    : user_share_(user_share),
       nudge_handler_(nudge_handler),
       weak_ptr_factory_(this) {
   for (size_t i = 0u; i < workers.size(); ++i) {
@@ -106,7 +108,7 @@ void ModelTypeRegistry::SetEnabledDirectoryTypes(
     if (it != directory_type_debug_info_emitter_map_.end()) {
       emitter = it->second;
     } else {
-      emitter = new DirectoryTypeDebugInfoEmitter(directory_, type,
+      emitter = new DirectoryTypeDebugInfoEmitter(directory(), type,
                                                   &type_debug_info_observers_);
       directory_type_debug_info_emitter_map_.insert(
           std::make_pair(type, emitter));
@@ -114,9 +116,9 @@ void ModelTypeRegistry::SetEnabledDirectoryTypes(
     }
 
     DirectoryCommitContributor* committer =
-        new DirectoryCommitContributor(directory_, type, emitter);
+        new DirectoryCommitContributor(directory(), type, emitter);
     DirectoryUpdateHandler* updater =
-        new DirectoryUpdateHandler(directory_, type, worker, emitter);
+        new DirectoryUpdateHandler(directory(), type, worker, emitter);
 
     // These containers take ownership of their contents.
     directory_commit_contributors_.push_back(committer);
@@ -141,6 +143,12 @@ void ModelTypeRegistry::ConnectType(
     std::unique_ptr<ActivationContext> activation_context) {
   DVLOG(1) << "Enabling an off-thread sync type: " << ModelTypeToString(type);
 
+  bool initial_sync_done =
+      activation_context->model_type_state.initial_sync_done();

[skym, 2016/10/21 15:34:19]

Do we have anything that watches for having metadata but initial_sync_done to
false and warns/complains?

[maxbogue, 2016/10/24 19:02:18]

Probably not? There could be something in SMTP::OnMetadataLoaded probs.

+  bool has_directory_data = directory()->InitialSyncEndedForType(type);

[skym, 2016/10/21 15:34:19]

So how expensive is this? If all 25 data types create their own read tx here on
startup, does that matter at all?

[maxbogue, 2016/10/24 19:02:17]

I mean, we might have to wait for the lock? But this is the sync thread so it
doesn't matter.

+  bool needs_migration = !initial_sync_done && has_directory_data;

[skym, 2016/10/21 15:34:19]

Should we have a finch feature param or something that lets us toggle if we want
to attempt to migrate certain data types? Or would we just turn off USS for that
type if something went wrong?

[maxbogue, 2016/10/24 19:02:17]

If something was crashing we'd just turn off USS for the type. If migration is
failing a lot we'd probably pause the rollout until we fix it, but since we fall
back to a normal download I don't think there would be a benefit to rolling
back.

[skym, 2016/10/25 15:48:11]

Okay, how do we pause the roll out? We don't really have a knob that allows to
keep everyone that has migrated to USS migrated, but don't move anyone new
forward, right? We'd stop increasing the % for the finch experiment, but there'd
still be people migrating as they restarted their clients.

It might make more sense to store some data in prefs to remember if this client
has been migrated instead of relying solely on finch params.

[maxbogue, 2016/10/26 23:26:15]

I've added a TODO for this.

+  bool trigger_initial_sync = !initial_sync_done && !has_directory_data;
+
   // Initialize Worker -> Processor communication channel.
   ModelTypeProcessor* type_processor = activation_context->type_processor.get();
 
@@ -149,8 +157,9 @@ void ModelTypeRegistry::ConnectType(
     cryptographer_copy = base::MakeUnique<Cryptographer>(*cryptographer_);
 
   std::unique_ptr<ModelTypeWorker> worker(new ModelTypeWorker(
-      type, activation_context->model_type_state, std::move(cryptographer_copy),
-      nudge_handler_, std::move(activation_context->type_processor)));
+      type, activation_context->model_type_state, trigger_initial_sync,
+      std::move(cryptographer_copy), nudge_handler_,
+      std::move(activation_context->type_processor)));
 
   // Initialize Processor -> Worker communication channel.
   std::unique_ptr<CommitQueue> commit_queue_proxy(new CommitQueueProxy(
@@ -166,10 +175,21 @@ void ModelTypeRegistry::ConnectType(
   commit_contributor_map_.insert(std::make_pair(type, worker.get()));
 
   // The container takes ownership.
+  ModelTypeWorker* worker_ptr = worker.get();
   model_type_workers_.push_back(std::move(worker));
 
   DCHECK(Intersection(GetEnabledDirectoryTypes(), GetEnabledNonBlockingTypes())
              .Empty());
+
+  if (needs_migration) {

[skym, 2016/10/21 15:34:19]

Can we get a TODO where you think we'd check to see if we can delete directory
data?

[maxbogue, 2016/10/24 19:02:18]

Done.

+    if (MigrateDirectoryData(type, user_share_, worker_ptr)) {

[skym, 2016/10/21 15:34:19]

Seems calling a static method that does lots of work makes it difficult to unit
test the registry. (Also would be good to have a test case for this).

[maxbogue, 2016/10/24 19:02:18]

Done: injected as a callback and test case on the registry added.

+      UMA_HISTOGRAM_BOOLEAN("Sync.USSMigrationResult", true);

[skym, 2016/10/21 15:34:19]

Any other metrics we care about? Time it takes? Memory it uses? Counts of
objects? Type that was migrated?

[maxbogue, 2016/10/24 19:02:18]

Time should be trivial because everything is in memory. Count of objects doesn't
really help us. The type seems important, so I've switched to using that.

+    } else {
+      UMA_HISTOGRAM_BOOLEAN("Sync.USSMigrationResult", false);
+      // Nudge for a real initial GetUpdates if migration failed.

[skym, 2016/10/21 15:34:19]

Can you elaborate on the state we're now in, how bad this is, etc.

[maxbogue, 2016/10/24 19:02:18]

Done.

+      nudge_handler_->NudgeForInitialDownload(type);
+    }
+  }
 }
 
 void ModelTypeRegistry::DisconnectType(ModelType type) {
@@ -205,7 +225,7 @@ ModelTypeSet ModelTypeRegistry::GetInitialSyncEndedTypes() const {
   // reported by directory and types reported by update handlers. We need to
   // refactor initialization and configuratrion flow to be able to only query
   // this set from update handlers.
-  ModelTypeSet result = directory_->InitialSyncEndedTypes();
+  ModelTypeSet result = directory()->InitialSyncEndedTypes();
   for (const auto& kv : update_handler_map_) {
     if (kv.second->IsInitialSyncEnded())
       result.Put(kv.first);