Let SyncBackupManager keep backup data in memory until shutdown. Only persist

Let SyncBackupManager keep backup data in memory until shutdown. Only persist
backup data to disk if shutdown is due to switching to sync.

BUG=362679
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289709

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289840

[Nicolas Zea, 2014-08-11 18:12:25]

https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
File sync/internal_api/public/test/test_internal_components_factory.h (right):

https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
sync/internal_api/public/test/test_internal_components_factory.h:16:
StorageOption expected_storage);
Why are there two storage options here? What is expected-storage for?

https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
File sync/syncable/deferred_on_disk_directory_backing_store.h (right):

https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
sync/syncable/deferred_on_disk_directory_backing_store.h:17: class
SYNC_EXPORT_PRIVATE DeferredOnDiskDirectoryBackingStore
Does this need to be an implementation of DirectoryBackingStore? In general we
try to avoid this kind of inheritance in favor of composition. Or perhaps in
this case we can just have the OnDiskDirectoryBackingStore expose some logic to
control when it creates/destroys the directory?

[haitaol1, 2014-08-11 18:49:09]

https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
File sync/internal_api/public/test/test_internal_components_factory.h (right):

https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
sync/internal_api/public/test/test_internal_components_factory.h:16:
StorageOption expected_storage);
|expected_storage| is the param that BuildDirectoryBackingStore() should be
called with. |option| decides the actual storage to use.

On 2014/08/11 18:12:25, Nicolas Zea wrote:
> Why are there two storage options here? What is expected-storage for?

https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
File sync/syncable/deferred_on_disk_directory_backing_store.h (right):

https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
sync/syncable/deferred_on_disk_directory_backing_store.h:17: class
SYNC_EXPORT_PRIVATE DeferredOnDiskDirectoryBackingStore
On 2014/08/11 18:12:25, Nicolas Zea wrote:
> Does this need to be an implementation of DirectoryBackingStore? In general we
> try to avoid this kind of inheritance in favor of composition. Or perhaps in
> this case we can just have the OnDiskDirectoryBackingStore expose some logic
to
> control when it creates/destroys the directory?

I feel it's cleaner to use a separate class given the differences between normal
on-disk db and deferred on-disk db. The deferred on-disk db actually doesn't
depend on existing db file, so can be loaded in a simple way. It also requires
switching db on first save.

[maniscalco, 2014-08-11 21:12:21]

On 2014/08/11 18:49:09, haitaol1 wrote:
>
https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
> File sync/internal_api/public/test/test_internal_components_factory.h (right):
> 
>
https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
> sync/internal_api/public/test/test_internal_components_factory.h:16:
> StorageOption expected_storage);
> |expected_storage| is the param that BuildDirectoryBackingStore() should be
> called with. |option| decides the actual storage to use.
> 
> On 2014/08/11 18:12:25, Nicolas Zea wrote:
> > Why are there two storage options here? What is expected-storage for?
> 
>
https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
> File sync/syncable/deferred_on_disk_directory_backing_store.h (right):
> 
>
https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
> sync/syncable/deferred_on_disk_directory_backing_store.h:17: class
> SYNC_EXPORT_PRIVATE DeferredOnDiskDirectoryBackingStore
> On 2014/08/11 18:12:25, Nicolas Zea wrote:
> > Does this need to be an implementation of DirectoryBackingStore? In general
we
> > try to avoid this kind of inheritance in favor of composition. Or perhaps in
> > this case we can just have the OnDiskDirectoryBackingStore expose some logic
> to
> > control when it creates/destroys the directory?
> 
> I feel it's cleaner to use a separate class given the differences between
normal
> on-disk db and deferred on-disk db. The deferred on-disk db actually doesn't
> depend on existing db file, so can be loaded in a simple way. It also requires
> switching db on first save.

What are the differences between the normal on-disk db and deferred on-disk db? 
I think there must be some important distinctions I'm missing.

[haitaol1, 2014-08-11 21:57:30]

On 2014/08/11 21:12:21, maniscalco wrote:
> On 2014/08/11 18:49:09, haitaol1 wrote:
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
> > File sync/internal_api/public/test/test_internal_components_factory.h
(right):
> > 
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
> > sync/internal_api/public/test/test_internal_components_factory.h:16:
> > StorageOption expected_storage);
> > |expected_storage| is the param that BuildDirectoryBackingStore() should be
> > called with. |option| decides the actual storage to use.
> > 
> > On 2014/08/11 18:12:25, Nicolas Zea wrote:
> > > Why are there two storage options here? What is expected-storage for?
> > 
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
> > File sync/syncable/deferred_on_disk_directory_backing_store.h (right):
> > 
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
> > sync/syncable/deferred_on_disk_directory_backing_store.h:17: class
> > SYNC_EXPORT_PRIVATE DeferredOnDiskDirectoryBackingStore
> > On 2014/08/11 18:12:25, Nicolas Zea wrote:
> > > Does this need to be an implementation of DirectoryBackingStore? In
general
> we
> > > try to avoid this kind of inheritance in favor of composition. Or perhaps
in
> > > this case we can just have the OnDiskDirectoryBackingStore expose some
logic
> > to
> > > control when it creates/destroys the directory?
> > 
> > I feel it's cleaner to use a separate class given the differences between
> normal
> > on-disk db and deferred on-disk db. The deferred on-disk db actually doesn't
> > depend on existing db file, so can be loaded in a simple way. It also
requires
> > switching db on first save.
> 
> What are the differences between the normal on-disk db and deferred on-disk
db? 
> I think there must be some important distinctions I'm missing.

Deferred on-disk db starts a new db in memory first and persists data to disk if
SaveChanges() is called.

[maniscalco, 2014-08-11 22:00:16]

On 2014/08/11 21:57:30, haitaol1 wrote:
> On 2014/08/11 21:12:21, maniscalco wrote:
> > On 2014/08/11 18:49:09, haitaol1 wrote:
> > >
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
> > > File sync/internal_api/public/test/test_internal_components_factory.h
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
> > > sync/internal_api/public/test/test_internal_components_factory.h:16:
> > > StorageOption expected_storage);
> > > |expected_storage| is the param that BuildDirectoryBackingStore() should
be
> > > called with. |option| decides the actual storage to use.
> > > 
> > > On 2014/08/11 18:12:25, Nicolas Zea wrote:
> > > > Why are there two storage options here? What is expected-storage for?
> > > 
> > >
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
> > > File sync/syncable/deferred_on_disk_directory_backing_store.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
> > > sync/syncable/deferred_on_disk_directory_backing_store.h:17: class
> > > SYNC_EXPORT_PRIVATE DeferredOnDiskDirectoryBackingStore
> > > On 2014/08/11 18:12:25, Nicolas Zea wrote:
> > > > Does this need to be an implementation of DirectoryBackingStore? In
> general
> > we
> > > > try to avoid this kind of inheritance in favor of composition. Or
perhaps
> in
> > > > this case we can just have the OnDiskDirectoryBackingStore expose some
> logic
> > > to
> > > > control when it creates/destroys the directory?
> > > 
> > > I feel it's cleaner to use a separate class given the differences between
> > normal
> > > on-disk db and deferred on-disk db. The deferred on-disk db actually
doesn't
> > > depend on existing db file, so can be loaded in a simple way. It also
> requires
> > > switching db on first save.
> > 
> > What are the differences between the normal on-disk db and deferred on-disk
> db? 
> > I think there must be some important distinctions I'm missing.
> 
> Deferred on-disk db starts a new db in memory first and persists data to disk
if
> SaveChanges() is called.

Which differs from the normal db, how?  Just that the normal one starts out on
disk?

[haitaol1, 2014-08-11 22:20:08]

On 2014/08/11 22:00:16, maniscalco wrote:
> On 2014/08/11 21:57:30, haitaol1 wrote:
> > On 2014/08/11 21:12:21, maniscalco wrote:
> > > On 2014/08/11 18:49:09, haitaol1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
> > > > File sync/internal_api/public/test/test_internal_components_factory.h
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/internal_api/public/tes...
> > > > sync/internal_api/public/test/test_internal_components_factory.h:16:
> > > > StorageOption expected_storage);
> > > > |expected_storage| is the param that BuildDirectoryBackingStore() should
> be
> > > > called with. |option| decides the actual storage to use.
> > > > 
> > > > On 2014/08/11 18:12:25, Nicolas Zea wrote:
> > > > > Why are there two storage options here? What is expected-storage for?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
> > > > File sync/syncable/deferred_on_disk_directory_backing_store.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/455023003/diff/1/sync/syncable/deferred_on_di...
> > > > sync/syncable/deferred_on_disk_directory_backing_store.h:17: class
> > > > SYNC_EXPORT_PRIVATE DeferredOnDiskDirectoryBackingStore
> > > > On 2014/08/11 18:12:25, Nicolas Zea wrote:
> > > > > Does this need to be an implementation of DirectoryBackingStore? In
> > general
> > > we
> > > > > try to avoid this kind of inheritance in favor of composition. Or
> perhaps
> > in
> > > > > this case we can just have the OnDiskDirectoryBackingStore expose some
> > logic
> > > > to
> > > > > control when it creates/destroys the directory?
> > > > 
> > > > I feel it's cleaner to use a separate class given the differences
between
> > > normal
> > > > on-disk db and deferred on-disk db. The deferred on-disk db actually
> doesn't
> > > > depend on existing db file, so can be loaded in a simple way. It also
> > requires
> > > > switching db on first save.
> > > 
> > > What are the differences between the normal on-disk db and deferred
on-disk
> > db? 
> > > I think there must be some important distinctions I'm missing.
> > 
> > Deferred on-disk db starts a new db in memory first and persists data to
disk
> if
> > SaveChanges() is called.
> 
> Which differs from the normal db, how?  Just that the normal one starts out on
> disk?

Normal one reads in db file if one exists. Deferred db always starts fresh.

[Nicolas Zea, 2014-08-13 20:32:05]

Couple more comments

https://codereview.chromium.org/455023003/diff/20001/sync/internal_api/test/t...
File sync/internal_api/test/test_internal_components_factory.cc (right):

https://codereview.chromium.org/455023003/diff/20001/sync/internal_api/test/t...
sync/internal_api/test/test_internal_components_factory.cc:62:
CHECK_EQ(expected_storage_, storage);
I prefer keeping the verification of expectations in the test itself, and just
exposing the storage that was used. Having this here starts changing what it is
the factory is trying to do.

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
File sync/syncable/deferred_on_disk_directory_backing_store.h (right):

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
sync/syncable/deferred_on_disk_directory_backing_store.h:17: class
SYNC_EXPORT_PRIVATE DeferredOnDiskDirectoryBackingStore
Update comment to be more clear about the changes from the normal on disk store.

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
File sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc:39:
TEST_F(DeferredOnDiskDirectoryBackingStoreTest, Load) {
nit: add comments about what is being tested and what the expectations are, here
and for other tests

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc:108: }  //
anonymous
s/anonymous/namespace

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc:109: }  //
syncable
s/syncable/namespace syncable

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc:110: }  //
syncer
here too

[haitaol1, 2014-08-14 01:30:35]

https://codereview.chromium.org/455023003/diff/20001/sync/internal_api/test/t...
File sync/internal_api/test/test_internal_components_factory.cc (right):

https://codereview.chromium.org/455023003/diff/20001/sync/internal_api/test/t...
sync/internal_api/test/test_internal_components_factory.cc:62:
CHECK_EQ(expected_storage_, storage);
On 2014/08/13 20:32:05, Nicolas Zea wrote:
> I prefer keeping the verification of expectations in the test itself, and just
> exposing the storage that was used. Having this here starts changing what it
is
> the factory is trying to do.

Done.

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
File sync/syncable/deferred_on_disk_directory_backing_store.h (right):

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
sync/syncable/deferred_on_disk_directory_backing_store.h:17: class
SYNC_EXPORT_PRIVATE DeferredOnDiskDirectoryBackingStore
On 2014/08/13 20:32:05, Nicolas Zea wrote:
> Update comment to be more clear about the changes from the normal on disk
store.

Done.

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
File sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc:39:
TEST_F(DeferredOnDiskDirectoryBackingStoreTest, Load) {
On 2014/08/13 20:32:05, Nicolas Zea wrote:
> nit: add comments about what is being tested and what the expectations are,
here
> and for other tests

Done.

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc:108: }  //
anonymous
On 2014/08/13 20:32:05, Nicolas Zea wrote:
> s/anonymous/namespace

Done.

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc:109: }  //
syncable
On 2014/08/13 20:32:05, Nicolas Zea wrote:
> s/syncable/namespace syncable

Done.

https://codereview.chromium.org/455023003/diff/20001/sync/syncable/deferred_o...
sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc:110: }  //
syncer
On 2014/08/13 20:32:05, Nicolas Zea wrote:
> here too

Done.

[Nicolas Zea, 2014-08-14 19:43:49]

lgtm

[haitaol1, 2014-08-14 20:22:04]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-08-14 20:23:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/455023003/40001

[commit-bot: I haz the power, 2014-08-14 21:41:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-14 22:10:50]

Committed patchset #3 (40001) as 289709

[miu, 2014-08-14 23:05:24]

A revert of this CL (patchset #3) has been created in
https://codereview.chromium.org/477813002/ by miu@chromium.org.

The reason for reverting is: sync_unit_tests broke on Linux dbg bot.

http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Tests%...

------------------------------------------------------------

[ RUN      ] SyncManagerTestWithMockScheduler.ReConfiguration

GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: Start(0)
NOTE: You can safely ignore the above warning unless this call should not
happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean
to enforce the call.  See
http://code.google.com/p/googlemock/wiki/CookBook#Knowing_When_to_Expect for
details.
Stack trace:
../../sync/internal_api/sync_manager_impl_unittest.cc:838: Failure
Value of: storage_used_
Actual: 2880154539
Expected: InternalComponentsFactory::STORAGE_ON_DISK
Which is: 1
[  FAILED  ] SyncManagerTestWithMockScheduler.ReConfiguration (34 ms)
[828/829] SyncManagerTestWithMockScheduler.ReConfiguration (34 ms)
[0814/160010:ERROR:kill_posix.cc(191)] Unable to terminate process group 6975:
No such process
[ RUN      ] SyncManagerTestWithMockScheduler.BasicConfiguration

GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: Start(0)
NOTE: You can safely ignore the above warning unless this call should not
happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean
to enforce the call.  See
http://code.google.com/p/googlemock/wiki/CookBook#Knowing_When_to_Expect for
details.
Stack trace:
../../sync/internal_api/sync_manager_impl_unittest.cc:838: Failure
Value of: storage_used_
Actual: 2880154539
Expected: InternalComponentsFactory::STORAGE_ON_DISK
Which is: 1
[  FAILED  ] SyncManagerTestWithMockScheduler.BasicConfiguration (29 ms)
[829/829] SyncManagerTestWithMockScheduler.BasicConfiguration (29 ms).

[haitaol1, 2014-08-14 23:33:55]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-08-14 23:37:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/455023003/60001

[commit-bot: I haz the power, 2014-08-15 05:43:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-15 14:24:48]

Committed patchset #4 (60001) as 289840