[sync] Add store version to model type store backend

[sync] Add store version to model type store backend

Add the ModelTypeStoreSchemaDescriptor protobuf, and add a basic
framework for performing migration. Add a Paranoid PRESUBMIT test to
make sure the record identifier for the db version won't be reused.

R=pavely@chromium.org

BUG=656737
Committed: https://crrev.com/4feacb1e9db9b1ecc40ce0531fa2e04e4ee8ee0e
Cr-Commit-Position: refs/heads/master@{#426900}

[Patrick Noland, 2016-10-17 18:37:42]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 18:38:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 19:50:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-17 19:50:07]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2016-10-17 21:31:41]

Pavel, PTAL

[maxbogue, 2016-10-18 00:17:51]

Caught a few small things while glancing through.

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
File components/sync/model_impl/model_type_store_backend.h (right):

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
components/sync/model_impl/model_type_store_backend.h:5: #ifndef
COMPONENTS_SYNC_CORE_MODEL_TYPE_STORE_BACKEND_H_
Bad merge?

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
components/sync/model_impl/model_type_store_backend.h:27: using
sync_pb::ModelTypeStoreSchemaDescriptor;
Never use "using" statements in header files; my understanding is that this puts
ModelTypeStoreSchemaDescriptor into the global namespace for anyone that
includes this file.

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
components/sync/model_impl/model_type_store_backend.h:131: #endif  //
COMPONENTS_SYNC_CORE_MODEL_TYPE_STORE_BACKEND_H_
Also here.

https://codereview.chromium.org/2426613003/diff/1/components/sync/protocol/mo...
File components/sync/protocol/model_type_store_schema_descriptor.proto (right):

https://codereview.chromium.org/2426613003/diff/1/components/sync/protocol/mo...
components/sync/protocol/model_type_store_schema_descriptor.proto:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
don't need the "(c) "

[pavely, 2016-10-18 18:58:26]

https://chromiumcodereview.appspot.com/2426613003/diff/1/components/sync/mode...
File components/sync/model_impl/model_type_store_backend.h (right):

https://chromiumcodereview.appspot.com/2426613003/diff/1/components/sync/mode...
components/sync/model_impl/model_type_store_backend.h:32: extern const char
kDBSchemaDescriptorRecordId[];
These two constants should not be needed outside of store backend
implementation. Could you move them to .cc file?

https://chromiumcodereview.appspot.com/2426613003/diff/1/components/sync/mode...
components/sync/model_impl/model_type_store_backend.h:112: bool
GetStoreDescriptor(ModelTypeStoreSchemaDescriptor* schema_descriptor);
We discussed this in person, it is better not to expose schema descriptor
without a good reason. Caller of Migrate for example cannot/shouldn't know what
desired_descriptor should look like as it might get some field values from
intermediate migrations. I propose to pass version number through function
parameters and only use schema descriptor for serialization.

https://chromiumcodereview.appspot.com/2426613003/diff/1/components/sync/prot...
File components/sync/protocol/protocol_sources.gni (right):

https://chromiumcodereview.appspot.com/2426613003/diff/1/components/sync/prot...
components/sync/protocol/protocol_sources.gni:31:
"//components/sync/protocol/model_type_store_schema_descriptor.proto",
Move this line to keep list sorted.

[Patrick Noland, 2016-10-19 23:12:09]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 23:12:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 00:51:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 00:51:53]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2016-10-20 18:29:55]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 18:30:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patrick Noland, 2016-10-20 18:38:20]

Patchset #2 (id:20001) has been deleted

[Patrick Noland, 2016-10-20 19:16:00]

Pavel, PTAL

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
File components/sync/model_impl/model_type_store_backend.h (right):

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
components/sync/model_impl/model_type_store_backend.h:5: #ifndef
COMPONENTS_SYNC_CORE_MODEL_TYPE_STORE_BACKEND_H_
On 2016/10/18 00:17:51, maxbogue wrote:
> Bad merge?

Done.

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
components/sync/model_impl/model_type_store_backend.h:27: using
sync_pb::ModelTypeStoreSchemaDescriptor;
On 2016/10/18 00:17:51, maxbogue wrote:
> Never use "using" statements in header files; my understanding is that this
puts
> ModelTypeStoreSchemaDescriptor into the global namespace for anyone that
> includes this file.

Done.

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
components/sync/model_impl/model_type_store_backend.h:32: extern const char
kDBSchemaDescriptorRecordId[];
On 2016/10/18 18:58:26, pavely wrote:
> These two constants should not be needed outside of store backend
> implementation. Could you move them to .cc file?

They're used in the unit test file as well. Is there a good way to make a
constant visible to the test class but not anywhere else?

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
components/sync/model_impl/model_type_store_backend.h:112: bool
GetStoreDescriptor(ModelTypeStoreSchemaDescriptor* schema_descriptor);
On 2016/10/18 18:58:25, pavely wrote:
> We discussed this in person, it is better not to expose schema descriptor
> without a good reason. Caller of Migrate for example cannot/shouldn't know
what
> desired_descriptor should look like as it might get some field values from
> intermediate migrations. I propose to pass version number through function
> parameters and only use schema descriptor for serialization.

Done.

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
components/sync/model_impl/model_type_store_backend.h:131: #endif  //
COMPONENTS_SYNC_CORE_MODEL_TYPE_STORE_BACKEND_H_
On 2016/10/18 00:17:51, maxbogue wrote:
> Also here.

Done.

https://codereview.chromium.org/2426613003/diff/1/components/sync/protocol/mo...
File components/sync/protocol/model_type_store_schema_descriptor.proto (right):

https://codereview.chromium.org/2426613003/diff/1/components/sync/protocol/mo...
components/sync/protocol/model_type_store_schema_descriptor.proto:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/10/18 00:17:51, maxbogue wrote:
> don't need the "(c) "

Done.

https://codereview.chromium.org/2426613003/diff/1/components/sync/protocol/pr...
File components/sync/protocol/protocol_sources.gni (right):

https://codereview.chromium.org/2426613003/diff/1/components/sync/protocol/pr...
components/sync/protocol/protocol_sources.gni:31:
"//components/sync/protocol/model_type_store_schema_descriptor.proto",
On 2016/10/18 18:58:26, pavely wrote:
> Move this line to keep list sorted.

Done.

[commit-bot: I haz the power, 2016-10-20 20:00:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 20:00:38]

Dry run: This issue passed the CQ dry run.

[pavely, 2016-10-20 21:33:52]

lgtm

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
File components/sync/model_impl/model_type_store_backend.h (right):

https://codereview.chromium.org/2426613003/diff/1/components/sync/model_impl/...
components/sync/model_impl/model_type_store_backend.h:32: extern const char
kDBSchemaDescriptorRecordId[];
On 2016/10/20 19:15:59, Patrick Noland wrote:
> On 2016/10/18 18:58:26, pavely wrote:
> > These two constants should not be needed outside of store backend
> > implementation. Could you move them to .cc file?
> 
> They're used in the unit test file as well. Is there a good way to make a
> constant visible to the test class but not anywhere else?

directory_backing_store_unittest circumvents this problem by placing this
"extern ..." declaration in test file.
Alternatively you can make these constants static const members of
ModelTypeStoreBackend. Either make them private and write accessor in test class
or make them public.

The issue with current approach is that these names become available to anyone
including this header file and names themselves don't indicate that they are
related to store.

https://codereview.chromium.org/2426613003/diff/40001/components/sync/model_i...
File components/sync/model_impl/model_type_store_backend.cc (right):

https://codereview.chromium.org/2426613003/diff/40001/components/sync/model_i...
components/sync/model_impl/model_type_store_backend.cc:97: return
Migrate(current_version, kLatestSchemaVersion);
Returning result of Migrate regardless of the value makes a potential for
someone in the future to write some more code after this "if" statement and not
notice this early return in successful case.

[Patrick Noland, 2016-10-20 23:41:56]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 23:43:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 01:53:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 01:53:36]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2016-10-21 17:02:22]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 17:02:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 18:34:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 18:34:25]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Patrick Noland, 2016-10-21 21:02:55]

The CQ bit was checked by pnoland@chromium.org

[Patrick Noland, 2016-10-21 21:02:56]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2426613003/#ps80001
(title: "[sync] Add store version to model type store backend")

[commit-bot: I haz the power, 2016-10-21 21:03:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 21:50:40]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-10-21 21:55:14]

Description was changed from

==========
[sync] Add store version to model type store backend

Add the ModelTypeStoreSchemaDescriptor protobuf, and add a basic
framework for performing migration. Add a Paranoid PRESUBMIT test to
make sure the record identifier for the db version won't be reused.

R=pavely@chromium.org

BUG=656737
==========

to

==========
[sync] Add store version to model type store backend

Add the ModelTypeStoreSchemaDescriptor protobuf, and add a basic
framework for performing migration. Add a Paranoid PRESUBMIT test to
make sure the record identifier for the db version won't be reused.

R=pavely@chromium.org

BUG=656737

Committed: https://crrev.com/4feacb1e9db9b1ecc40ce0531fa2e04e4ee8ee0e
Cr-Commit-Position: refs/heads/master@{#426900}
==========

[commit-bot: I haz the power, 2016-10-21 21:55:15]

Patchset 4 (id:??) landed as
https://crrev.com/4feacb1e9db9b1ecc40ce0531fa2e04e4ee8ee0e
Cr-Commit-Position: refs/heads/master@{#426900}