[Sync] Implementation of ModelTypeStoreBackend

[Sync] Implementation of ModelTypeStoreBackend

I'm adding implementation of ModelTypeStoreBackend. The object lives on
backend thread and performs actual calls to leveldb. ModelTypeStoreImpl
will use interface of backend to work with database.

Implementation of ModelTypeStoreImpl methods that work with this backend
will be landed in separate CL.

R=stanisc@chromium.org
BUG=517663
TEST=Covered by sync_unit_tests. No externally observable behavior.

Committed: https://crrev.com/d88f80efd9b66130adf721f01ff355db57829c7e
Cr-Commit-Position: refs/heads/master@{#359442}

[stanisc, 2015-11-09 22:26:38]

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/model_typ...
File sync/internal_api/model_type_store_backend.cc (right):

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/model_typ...
sync/internal_api/model_type_store_backend.cc:50: return
ModelTypeStore::Result::UNSPECIFIED_ERROR;
I wonder if this code should somehow surface the underlying leveldb::Status. How
about including the string representation of the status into
ModelTypeStore::Result. That could help with leveldb specific error diagnostics.
It is OK to address than in a separate change.

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/model_typ...
sync/internal_api/model_type_store_backend.cc:96:
ModelTypeStore::Record(key.ToString(), iter->value().ToString()));
So right here in this line of code each string will be copied twice. Is there
any way to avoid that?

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/public/mo...
File sync/internal_api/public/model_type_store_backend.h (right):

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/public/mo...
sync/internal_api/public/model_type_store_backend.h:37: void
PassEnvOwnership(scoped_ptr<leveldb::Env> env);
Since this function is on the backend, should it be called TakeEnvOwnership.
Pass implies that it is passed from this object elsewhere.

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/public/mo...
sync/internal_api/public/model_type_store_backend.h:43: ModelTypeStore::Result
Init(const std::string& path, leveldb::Env* env);
It isn't quite clear why passing |env| here is necessary? Should it be passed
with the method above instead? Perhaps you should move your comments on the
ownership from |env_| to the method above.

[pavely, 2015-11-11 01:13:46]

PTAL.

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/model_typ...
File sync/internal_api/model_type_store_backend.cc (right):

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/model_typ...
sync/internal_api/model_type_store_backend.cc:50: return
ModelTypeStore::Result::UNSPECIFIED_ERROR;
On 2015/11/09 22:26:38, stanisc wrote:
> I wonder if this code should somehow surface the underlying leveldb::Status.
How
> about including the string representation of the status into
> ModelTypeStore::Result. That could help with leveldb specific error
diagnostics.
> It is OK to address than in a separate change.

I will need to solve the whole diagnostic question for USS store. It should
include logging, UMA histograms, about:sync support. I'll do that part
separately in multiple changes. At the moment I'm focusing on basic working
implementation of store.

One of the work items will be to provide logger in leveldb env. This will output
leveldb messages to log.

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/model_typ...
sync/internal_api/model_type_store_backend.cc:96:
ModelTypeStore::Record(key.ToString(), iter->value().ToString()));
On 2015/11/09 22:26:38, stanisc wrote:
> So right here in this line of code each string will be copied twice. Is there
> any way to avoid that?

Ideally vector::emplace_back would solve this, but I don't think it is allowed
yet.
Alternatively I could pass around vector of scoped_ptrs to Record. This would
avoid copy of string objects, but cost additional allocation overhead. Right now
Record's size is two pointers and string copies don't incur actual copy of
contents (at least on linux debug build). I think it should be fine.

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/public/mo...
File sync/internal_api/public/model_type_store_backend.h (right):

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/public/mo...
sync/internal_api/public/model_type_store_backend.h:37: void
PassEnvOwnership(scoped_ptr<leveldb::Env> env);
On 2015/11/09 22:26:38, stanisc wrote:
> Since this function is on the backend, should it be called TakeEnvOwnership.
> Pass implies that it is passed from this object elsewhere.

Done.

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/public/mo...
sync/internal_api/public/model_type_store_backend.h:43: ModelTypeStore::Result
Init(const std::string& path, leveldb::Env* env);
On 2015/11/09 22:26:38, stanisc wrote:
> It isn't quite clear why passing |env| here is necessary? Should it be passed
> with the method above instead? Perhaps you should move your comments on the
> ownership from |env_| to the method above.

I've adjusted comment.

[stanisc, 2015-11-11 01:52:20]

lgtm with a comment

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/model_typ...
File sync/internal_api/model_type_store_backend.cc (right):

https://codereview.chromium.org/1436573002/diff/1/sync/internal_api/model_typ...
sync/internal_api/model_type_store_backend.cc:96:
ModelTypeStore::Record(key.ToString(), iter->value().ToString()));
On 2015/11/11 01:13:46, pavely wrote:
> On 2015/11/09 22:26:38, stanisc wrote:
> > So right here in this line of code each string will be copied twice. Is
there
> > any way to avoid that?
> 
> Ideally vector::emplace_back would solve this, but I don't think it is allowed
> yet.
> Alternatively I could pass around vector of scoped_ptrs to Record. This would
> avoid copy of string objects, but cost additional allocation overhead. Right
now
> Record's size is two pointers and string copies don't incur actual copy of
> contents (at least on linux debug build). I think it should be fine. 

My understanding that string data isn't shared anymore when one string is copied
into another - COW strings are not C++ 11 compliant. 
So this will indeed make two full copies of each string or perhaps even 3
copies:
1) ToString() - copy the data from Slice to String
2) Record constructor - copy the input string to the member field (unless there
is string constructor that supports move semantics).
3) Copy strings as a part of Record when inserting into the vector.

There is no need to make any changes in this patch but I encourage you to look
at the generated code and/or run this under debugger to understand how much data
is actually copied. Here are some ideas to consider for later:
1) Make an overloaded Record constructor that accepts Slice arguments
2) Use ScopedVector instead of regular vector and/or pass the entire vector
wrapped into scoped_ptr.

[pavely, 2015-11-11 21:12:08]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2015-11-11 21:13:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1436573002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1436573002/20001

[commit-bot: I haz the power, 2015-11-11 21:23:36]

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

[commit-bot: I haz the power, 2015-11-11 21:23:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pavely, 2015-11-11 21:26:39]

Description was changed from

==========
[Sync] Implementation of ModelTypeStoreBackend

I'm adding implementation of ModelTypeStoreBackend. The object lives on
backend thread and performs actual calls to leveldb. ModelTypeStoreImpl
will use interface of backend to work with database.

Implementation of ModelTypeStoreImpl methods that work with this backend
will be landed in separate CL.

R=stanisc@chromium.org
BUG=517663
TEST=Covered by sync_unit_tests. No externally observable behavior.
==========

to

==========
[Sync] Implementation of ModelTypeStoreBackend

I'm adding implementation of ModelTypeStoreBackend. The object lives on
backend thread and performs actual calls to leveldb. ModelTypeStoreImpl
will use interface of backend to work with database.

Implementation of ModelTypeStoreImpl methods that work with this backend
will be landed in separate CL.

R=stanisc@chromium.org
BUG=517663
TEST=Covered by sync_unit_tests. No externally observable behavior.
==========

[pavely, 2015-11-11 21:26:39]

pavely@chromium.org changed reviewers:
+ cmumford@chromium.org

[pavely, 2015-11-11 21:32:16]

cmumford@ : I'm adding dependency to third_party/leveldatabase. Please review
changes in DEPS file.

[cmumford, 2015-11-12 00:26:36]

DEPS l-g-t-m to me. I took the liberty of also reviewing the code that calls
leveldb and added a few comments there too.

https://codereview.chromium.org/1436573002/diff/20001/sync/internal_api/model...
File sync/internal_api/model_type_store_backend.cc (right):

https://codereview.chromium.org/1436573002/diff/20001/sync/internal_api/model...
sync/internal_api/model_type_store_backend.cc:43: options.create_if_missing =
true;
Recommend adding "options.paranoid_checks = true". It's low cost, and will avoid
some runtime failures by catching corruptions early.

https://codereview.chromium.org/1436573002/diff/20001/sync/internal_api/model...
sync/internal_api/model_type_store_backend.cc:47: leveldb::Status status =
leveldb::DB::Open(options, path, &db_raw);
I recommend that if (status.IsCorruption()) that you either call RepairDB, or
delete the folder. Once it gets in this state (and it will) then you will have a
forever failing db. See issue 549177 for an example. Check out r358722 for my
fix to that bug for an example solution.

https://codereview.chromium.org/1436573002/diff/20001/sync/internal_api/model...
sync/internal_api/model_type_store_backend.cc:52: db_.reset(db_raw);
Recommend following up with a CL to add some UMA stats. If/when you add them
please let me know and I'll add them to my dashboard that monitors all leveldb
users in Chrome.

https://codereview.chromium.org/1436573002/diff/20001/sync/internal_api/model...
sync/internal_api/model_type_store_backend.cc:101: return iter->status().ok() ?
ModelTypeStore::Result::SUCCESS
FYI, it is possible to have corrupted db's which can be successfully opened w/o
returning a corruption error - only to be caught later on. It's a pain in the
butt, but you may want to make your implementation able to deal this this at
some point.

[pavely, 2015-11-12 22:29:12]

Christopher, PTAL.

https://codereview.chromium.org/1436573002/diff/20001/sync/internal_api/model...
File sync/internal_api/model_type_store_backend.cc (right):

https://codereview.chromium.org/1436573002/diff/20001/sync/internal_api/model...
sync/internal_api/model_type_store_backend.cc:43: options.create_if_missing =
true;
On 2015/11/12 00:26:36, cmumford wrote:
> Recommend adding "options.paranoid_checks = true". It's low cost, and will
avoid
> some runtime failures by catching corruptions early.

Done.

https://codereview.chromium.org/1436573002/diff/20001/sync/internal_api/model...
sync/internal_api/model_type_store_backend.cc:47: leveldb::Status status =
leveldb::DB::Open(options, path, &db_raw);
On 2015/11/12 00:26:36, cmumford wrote:
> I recommend that if (status.IsCorruption()) that you either call RepairDB, or
> delete the folder. Once it gets in this state (and it will) then you will have
a
> forever failing db. See issue 549177 for an example. Check out r358722 for my
> fix to that bug for an example solution.

I want to make these changes separately. I've filed http://crbug.com/555094 to
track this work.

https://codereview.chromium.org/1436573002/diff/20001/sync/internal_api/model...
sync/internal_api/model_type_store_backend.cc:52: db_.reset(db_raw);
On 2015/11/12 00:26:36, cmumford wrote:
> Recommend following up with a CL to add some UMA stats. If/when you add them
> please let me know and I'll add them to my dashboard that monitors all leveldb
> users in Chrome.

I will do that in separate CL.

https://codereview.chromium.org/1436573002/diff/20001/sync/internal_api/model...
sync/internal_api/model_type_store_backend.cc:101: return iter->status().ok() ?
ModelTypeStore::Result::SUCCESS
On 2015/11/12 00:26:36, cmumford wrote:
> FYI, it is possible to have corrupted db's which can be successfully opened
w/o
> returning a corruption error - only to be caught later on. It's a pain in the
> butt, but you may want to make your implementation able to deal this this at
> some point.

Same as above. I'll handle diagnistic, corruption detection and repair in
separate CL.

[cmumford, 2015-11-12 22:52:14]

lgtm

[pavely, 2015-11-12 22:53:43]

The CQ bit was checked by pavely@chromium.org

[pavely, 2015-11-12 22:53:43]

The patchset sent to the CQ was uploaded after l-g-t-m from stanisc@chromium.org
Link to the patchset: https://codereview.chromium.org/1436573002/#ps60001
(title: "Add paranoid_checks")

[commit-bot: I haz the power, 2015-11-12 22:54:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1436573002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1436573002/60001

[commit-bot: I haz the power, 2015-11-12 23:55:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-12 23:57:23]

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