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"

[tim (not reviewing), 2012/04/25 06:48:13]

Can we add a commit_unittest to test high level BuildAndPostCommits
functionality specifically?

[rlarocque, 2012/04/25 18:13:22]

This function is well covered by the SyncerTests.  Those tests have not been
lost in the refactor.

If we were starting from nothing, I agree that it would make more sense to write
those tests against this function rather than the syncer.  However, I don't see
a need to rewrite them right now.

+
+#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(SyncSession* session) {
+  StatusController* status_controller = session->mutable_status_controller();
+  syncable::Directory* dir = session->context()->directory();
+  size_t batch_size = session->context()->max_commit_batch_size();
+  for ( ; ; ) {

[tim (not reviewing), 2012/04/25 06:48:13]

What is this loop for?  It's not obvious to me.  Depending on what this is for
it might be better to have a while loop that explains its own existence?

[rlarocque, 2012/04/25 18:13:22]

The initial idea was to loop over the set of all commit batches.  I think you
have a point, though.  This is more analogous to popping items of a stack one by
one than iterating through a list, since we are modifying the container as we
iterate.  A while loop would be clearer.  

I think it will still have to be a "while(1)" loop, though.  We don't know until
line 42 whether or not we should continue iterating.  I don't see any way to
squash all the lines between here and there into a single expression.

+    sessions::OrderedCommitSet commit_set(session->routing_info());
+    ClientToServerMessage commit_message;
+    ClientToServerResponse commit_response;
+
+    {
+      TRACE_EVENT0("sync", "BuildCommit");
+      WriteTransaction trans(FROM_HERE, SYNCER, dir);
+      sessions::ScopedSetSessionWriteTransaction set_trans(session, &trans);
+      GetCommitIdsCommand get_commit_ids_command(batch_size, &commit_set);
+      get_commit_ids_command.Execute(session);
+
+      DVLOG(1) << "Found " << commit_set.Size()
+          << " item(s) to commit in this batch.";
+
+      // Exit if we have nothing left to commit.  This is the normal exit point.
+      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,
+                                                 &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();
+    }
+  }
+}

[tim (not reviewing), 2012/04/25 06:48:13]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Write_...

One concern I have is that I think stepwise refining this routine now or in the
future is more cumbersome than the average Chrome component.  I wonder if this
might be better written as a class with a single public entry point, and as many
private helpers as needed that can all access (for example) the state in your
outermost scope.  

The motivation is that while I think we should try to break this up already (it
does exceed the 40-line style guide guilt-laying rule!), if we ever need to add
or expand some of the code above we're doomed to keep appending to this
function, float helper functions in the namespace for the world to (ab)use, or
add a bunch of local / file static functions. The latter is the probably the
best option, but even though logically all the steps share state we'll need to
plumb that list to each function, and so I think 'append' will happen most of
the time. Unless it is the natural / easy choice to do something else.

e.g. a case I've hit before is where I want to add a little code to do a bunch
of sanity-checks / DCHECKs. I'll be tempted to push it into a function I can
reuse, to keep the main routine concise. But if I have to pass a few lines worth
of arguments to that function each time, it's probably not worth it and I won't
bother.  

[tim (not reviewing), 2012/04/25 06:48:13]

Does this address / maintain somehow the early-exit-requested behavior between
commands?

[rlarocque, 2012/04/25 18:13:22]

I believe it does.  If we receive an early exit request, the
ServerConnectionManager will be told to abort any pending network requests and
return an error.  This should allow us to exit the function fairly quickly.

[rlarocque, 2012/04/25 18:13:22]

This is exactly the sort of thing I want to encourage.  The alternative is to
store data as members that all these functions would have access to, making it
unclear which functions have access to which data.  Classes are meant to hide
the representation of data from the outside world, not to expose that data to a
set of functions on the inside.  
If you need to pass a few lines worth of arguments to the function, and the
function is used from exactly one context, then I think it should be inlined. 
Moving it into a separate function won't make the code any less complex,
especially if this new function has access to exactly the same data as it did in
the original function context.  

=====

To bring this back to the code in question, here's a list of refactorings I had
in mind that should make this function less complex:
- Convert GetCommitIdsCommand and BuildCommitCommand from SyncerCommands into
functions.  That should reduce some boilerplate code (ie. the instantiation of
these objects before we call their functions), and eventually reduce our
reliance on the SyncSession.
- Replace last_process_commit_response and last_post_commit_response with a
single commit_response indicator.  This function could then return a single
value and let the Syncer handle setting it.
- Move the ModelNeutral part of ProcessCommitResponse into a helper function in
this context so we can simplify some of the early-exit logic (ie. no more
running ProcessCommitResponse after a commit fails).

My point here is that there are some simple steps we can take to improve this
function.  I just don't want this diff to get any bigger than it already is.  

[tim (not reviewing), 2012/04/26 06:47:54]

Yeah, I'm not saying I wholesale disagree with that. But status_controller, dir,
and batch_size are scoped for every part of this function to use as you've
written it. So I don't think you would change much besides except
ease-of-creating subfunctions by making those three member variables of a class.

[rlarocque, 2012/05/08 00:48:32]

On that point, I agree.  The batch_size and dir definitions were written when I
was trying to implement this in a different way.  They're not required for the
current approach, and the code is clearer if I move or remove them.  

That leaves status_controller, which is useful as a way to save typing when
saving the results of the SyncerCommands.  I intend to remove those status
controller members in a follow-up patch, so I won't need the status_controller
definition for much longer.

======

Although there is no current need for class-scope variables, I will
pre-emptively embed this function inside a "class Committer".  If I'm right
(that we don't need class-scope variables) the Committer class will remain
memberless for the foreseeable future, no harm done.  If I'm wrong, then it's
easy to add members as required.  

+
+}  // namespace browser_sync