sync: Loop committing items without downloading updates (v2)

sync/engine/commit.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/commit.h"
+
+#include "base/debug/trace_event.h"
+#include "sync/engine/build_commit_command.h"
+#include "sync/engine/get_commit_ids_command.h"
+#include "sync/engine/process_commit_response_command.h"
+#include "sync/engine/syncer_proto_util.h"
+#include "sync/sessions/sync_session.h"
+
+using syncable::SYNCER;
+using syncable::WriteTransaction;
+
+namespace browser_sync {
+
+using sessions::SyncSession;
+using sessions::StatusController;
+
+SyncerError BuildAndPostCommits(sessions::SyncSession* session) {
+  StatusController* status_controller = session->mutable_status_controller();
+  size_t items_to_commit_count = 0;

[tim (not reviewing), 2012/05/11 18:42:16]

Perhaps this should be inside the loop.

[rlarocque, 2012/05/14 23:10:43]

I tried.  It won't work.  The while() condition of the do loop doesn't have
access to that scope.

+  const size_t batch_size = session->context()->max_commit_batch_size();
+  do {
+    sessions::OrderedCommitSet commit_set(session->routing_info());
+    ClientToServerMessage commit_message;
+    ClientToServerResponse commit_response;
+
+    {
+      TRACE_EVENT0("sync", "BuildCommit");
+      WriteTransaction trans(FROM_HERE, SYNCER,
+                             session->context()->directory());
+      sessions::ScopedSetSessionWriteTransaction set_trans(session, &trans);
+      GetCommitIdsCommand get_commit_ids_command(batch_size, &commit_set,

[tim (not reviewing), 2012/05/11 18:42:16]

This line is kind of taunting me to suggest an optimization where we don't need
to re-scan on each iteration, but I wonder how bad it is in practice (as you
mentioned yesterday, it's a whole lot better now without the extra GU in the
loop!).

Most of the ideas I can think of that would help are not exactly trivial. We
could snapshot each commit batch, and only re-scan the directory + filter if the
commit response changed anything that might affect IsEntryReadyForCommit
(encryption, throttled types). It's a bit tricky. We could keep an index on
"unready" items... humph. It's probably not worth spending too much thought on
right now. I doubt it really affects the steady state anyway (though it
certainly affects re-encryption and first-time-sync).

[rlarocque, 2012/05/14 23:10:43]

I agree that we can do better.  At first, I tried being a bit more ambitious
here and implementing some of those optimizations.  It turns out that there are
some non-obvious difficulties, so I gave up and decided to mimic the "classic"
behaviour.  

I would be interested in revisiting this code after this CL lands.

+                                                 &items_to_commit_count);
+      get_commit_ids_command.Execute(session);
+
+      DVLOG(1) << "Found " << commit_set.Size()
+          << " item(s) to commit in this batch.";
+
+      if (commit_set.Empty())
+        return SYNCER_OK;
+
+      BuildCommitCommand build_commit_command(commit_set,
+                                              &commit_message);
+      build_commit_command.Execute(session);
+    } // Time for some network IO.  Release the lock.
+
+    DVLOG(1) << "Sending a commit message";
+    TRACE_EVENT_BEGIN0("sync", "PostCommit");
+    status_controller->set_last_post_commit_result(
+      SyncerProtoUtil::PostClientToServerMessage(commit_message,

[tim (not reviewing), 2012/05/11 18:42:16]

As general practice all over the code, we ought to involve the session delegate
any time we are considering re-POSTing.

Previously, there was a chance for the scheduler to intercept between subsequent
GUs (admittedly, only by early exit requested) and subsequent [GU Commit] units,
either for backoff or early-exit requested.  Now, it looks like the atomic unit
is [GU Commit*]. I'm worried about having a tight loop around
PostClientToServerMessage without any interceptability at all. We've been burned
by tight loops in the past and it makes it harder to implement features related
to rate limiting since there's no single choke point.

For now the call could simply test early exit requested, but could be extended
in the future (for example if we chose to implement a doomsday-avoidance feature
like Lingesh proposed.)

It would also be nice to have a test verifying we boot out of here in certain
situations, depending on the delegate.

[rlarocque, 2012/05/14 23:10:43]

The ServerConnectionManager would handle that sort of thing.  As I've written
it, this code implicitly relies on the SCM for early exit.  I'd argue that the
SCM (or, better yet, URLFetcher) would be a much better place to implement
doomsday-avoidance.  I think almost everything that should be done for every
POST would be better off if it were implemented as close to the POST as
possible.
I thought this code had inherited some of the existing tests for that scenario. 
I'll look into it.

[tim (not reviewing), 2012/05/15 22:25:00]

My main point was that in general the delegate is going to have app-layer data /
knowledge that the http layer SCM won't, so it typically needs to be consulted
from wherever the decision to post or not is being made.

As long as we don't diverge too far from what we have today (e.g. with exit
requested status) we can deal with this later. In the meantime it'd be nice to
maintain peace of mind in that we're not regressing :)

+                                                 &commit_response,
+                                                 session));
+    TRACE_EVENT_END0("sync", "PostCommit");
+
+    // ProcessCommitResponse includes some code that cleans up after a failure
+    // to post a commit message, so we must run it regardless of whether or not
+    // the commit succeeds.
+
+    TRACE_EVENT_BEGIN0("sync", "ProcessCommitResponse");
+    ProcessCommitResponseCommand process_response_command(
+        commit_set, commit_message, commit_response);
+    status_controller->set_last_process_commit_response_result(
+            process_response_command.Execute(session));
+    TRACE_EVENT_END0("sync", "ProcessCommitResponse");
+
+    // Exit early if either the commit or the response processing failed.
+    if (status_controller->last_post_commit_result() != SYNCER_OK) {
+      return status_controller->last_post_commit_result();
+    }
+    if (status_controller->last_process_commit_response_result() != SYNCER_OK) {
+      return status_controller->last_process_commit_response_result();
+    }
+  } while (items_to_commit_count > batch_size);

[rlarocque, 2012/05/11 00:52:16]

I realized after posting this patch that there is a strange edge case here.

Consider crbug.com/94675 in this context.  If we delete a folder that contains
30 items, the items_to_commit_count will be 31 (30 items + the folder).  If
batch_size is 25, then this condition will be true and we will return to the top
of the loop.  

This would be not entirely correct.  The deletions will be sent over the network
as a single folder delete, which would fit in a single batch.  After a single
commit, we would be fully synced.  The extra iteration would be unnecessary.  

There's no harm in this, since we will exit on the next iteration on line 44. 
It's not very obvious to people reading the code, though, so I should add a
comment documenting this behaviour.

[tim (not reviewing), 2012/05/11 18:42:16]

A comment explaining what happens if local changes happen during a cycle might
be nice too (i.e. a nudge will get scheduled).

+
+  return SYNCER_OK;
+}
+
+}  // namespace browser_sync