Implementation of OnDiskAttachmentStore.

sync/internal_api/attachments/on_disk_attachment_store.cc

Index: sync/internal_api/attachments/on_disk_attachment_store.cc
diff --git a/sync/internal_api/attachments/on_disk_attachment_store.cc b/sync/internal_api/attachments/on_disk_attachment_store.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0172027cac21a675d3b154a6fec9afd47eb640dc
--- /dev/null
+++ b/sync/internal_api/attachments/on_disk_attachment_store.cc
@@ -0,0 +1,180 @@
+// Copyright 2014 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/internal_api/public/attachments/on_disk_attachment_store.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/memory/scoped_ptr.h"
+#include "sync/protocol/attachments.pb.h"
+#include "third_party/leveldatabase/src/include/leveldb/db.h"
+#include "third_party/leveldatabase/src/include/leveldb/options.h"
+#include "third_party/leveldatabase/src/include/leveldb/slice.h"
+#include "third_party/leveldatabase/src/include/leveldb/status.h"
+
+namespace syncer {
+
+namespace {
+
+const char kAttachmentDataStart[] = "attachment_data1-";

[maniscalco, 2014/10/14 21:03:18]

WDYT about just "data-" ?  Seems like we'd benefit from a short prefix since the
DB will store it over and over.

Also, what's the 1 for?  I guess I was expecting the entire DB to be versioned,
not each key/value pair.  Let's chat about this one in person.

[maniscalco, 2014/10/15 16:16:36]

As we talked about in person...

- an alternative to using the start/end approach is to use a key prefix.  When
iterating keys, when you find a key whose prefix does not match the one you
started with, you know you've seen all the records of the type you were looking
for.

[pavely, 2014/10/16 22:13:29]

Done.

+// kAttachmentDataEnd will be used for enumeration. It is commented out since
+// compiler complains that it isn't used.
+// const char kAttachmentDataEnd[] = "attachment_data2-";

[maniscalco, 2014/10/14 21:03:19]

Please remove this.  Let's avoid checking in any commented out code.

[pavely, 2014/10/16 22:13:29]

Done.

+
+}  // namespace
+
+OnDiskAttachmentStore::OnDiskAttachmentStore(
+    const base::FilePath& path,
+    const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner)
+    : path_(path),

[maniscalco, 2014/10/15 16:16:36]

As we talked about in person...

- in the future, we may change from leveldb to some other data store.  We can
make doing so easier if we make the leveldb dir (the one we pass to
level::DB::Open) a subdir under the AttachmentStore dir (e.g.
/my-attachment-store/level-db/).  Future version of AS can look in the
AttachmentStore dir to figure out what kind of AS they are using (no need to
create an external description file at this point).

- let's ensure that old code pointed at a newer db, will recognize that it
cannot possibly understand the new db and refuse to open it.  We talked about
doing this using a well known metadata record (proto value) in the store.

[pavely, 2014/10/16 22:13:29]

Done with creating leveldb in subdir. Will implement metadata record in separate
change.

[maniscalco, 2014/10/17 00:00:19]

SGTM

+      callback_task_runner_(callback_task_runner) {
+}
+
+OnDiskAttachmentStore::~OnDiskAttachmentStore() {
+}
+
+void OnDiskAttachmentStore::Load(const LoadCallback& callback) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!db_);
+  Result result_code = UNSPECIFIED_ERROR;
+
+  leveldb::Status status;
+  leveldb::DB* db;
+  leveldb::Options options;
+  options.create_if_missing = true;

[maniscalco, 2014/10/15 16:16:36]

As we talked about in person...

- what does leveldb do if we point it as some non-empty dir that contains
non-leveldb files?  Any chance we could do harm by pointing an AttachmentStore
at a path exists and contains non-AS/non-leveldb files?  Would be great to have
the property that the worst you can do by giving a wrong path is create some new
files on disk that won't get cleaned up.

[pavely, 2014/10/16 22:13:29]

During normal operations leveldb only affects files with names in format it
recognizes, everything else stays intact.
There is DestroyDB function that deletes leveldb files individually, but then
calls env->DeleteDir. Posix implementation of it will keep directory intact if
it still has files, but there is no requirement explained on env.h for this
function.

[maniscalco, 2014/10/17 00:00:19]

Acknowledged.

+  // TODO(pavely): Consider adding info_log, block_cache and filter_policy to

[maniscalco, 2014/10/14 21:03:18]

Can you please create crbugs for TODOs like this and reference the bug number in
the TODO (like we do for other attachment related TODOs).

[pavely, 2014/10/16 22:13:29]

Done.

+  // options.
+  status = leveldb::DB::Open(options, path_.AsUTF8Unsafe(), &db);

[maniscalco, 2014/10/14 21:03:18]

Any reason not to define status here instead of above?

[pavely, 2014/10/16 22:13:29]

Done.

+  if (!status.ok()) {
+    DVLOG(1) << "DB::Open failed: status=" << status.ToString()
+             << ", path=" << path_.AsUTF8Unsafe();
+  } else {
+    db_.reset(db);
+    result_code = SUCCESS;
+  }
+
+  callback_task_runner_->PostTask(FROM_HERE, base::Bind(callback, result_code));
+}
+
+void OnDiskAttachmentStore::Read(const AttachmentIdList& ids,
+                                 const ReadCallback& callback) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(db_);
+  scoped_ptr<AttachmentMap> result_map(new AttachmentMap());
+  scoped_ptr<AttachmentIdList> unavailable_attachments(new AttachmentIdList());
+
+  leveldb::ReadOptions read_options;
+  // Attachment content is typically large and only read once. Don't cache it on
+  // db level.
+  read_options.fill_cache = false;
+  read_options.verify_checksums = true;
+
+  AttachmentIdList::const_iterator iter = ids.begin();
+  AttachmentIdList::const_iterator end = ids.end();

[maniscalco, 2014/10/14 21:03:18]

Opportunity for const?

[pavely, 2014/10/16 22:13:29]

Done.

+  for (; iter != end; ++iter) {
+    std::string key;
+    FormatAttachmentDataKey(*iter, &key);

[maniscalco, 2014/10/14 21:03:18]

Why take the string as an output param?  Why not just return std::string by
value and rely-on/hope-for NRVO?

[pavely, 2014/10/16 22:13:29]

Done.

+    std::string data_str;
+    leveldb::Status status = db_->Get(read_options, key, &data_str);

[maniscalco, 2014/10/14 21:03:18]

Opportunity for const?

[pavely, 2014/10/16 22:13:29]

Done.

+    if (!status.ok()) {
+      DVLOG(1) << "DB::Get failed: status=" << status.ToString();
+      unavailable_attachments->push_back(*iter);
+      continue;
+    }
+    scoped_refptr<base::RefCountedMemory> data =
+        base::RefCountedString::TakeString(&data_str);
+    Attachment attachment = Attachment::CreateWithId(*iter, data);
+    result_map->insert(std::make_pair(*iter, attachment));
+  }
+
+  Result result_code =
+      unavailable_attachments->empty() ? SUCCESS : UNSPECIFIED_ERROR;
+  callback_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(callback,
+                 result_code,
+                 base::Passed(&result_map),
+                 base::Passed(&unavailable_attachments)));
+}
+
+void OnDiskAttachmentStore::Write(const AttachmentList& attachments,
+                                  const WriteCallback& callback) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(db_);
+  Result result_code = SUCCESS;
+
+  leveldb::ReadOptions read_options;
+  read_options.fill_cache = false;
+  read_options.verify_checksums = true;
+
+  leveldb::WriteOptions write_options;
+  write_options.sync = true;
+
+  AttachmentList::const_iterator iter = attachments.begin();
+  AttachmentList::const_iterator end = attachments.end();
+  for (; iter != end; ++iter) {
+    leveldb::Status status;
+    std::string key;
+    FormatAttachmentDataKey(iter->GetId(), &key);
+
+    std::string data_str;
+    // This read is expensive. When I add metadata records this read will target
+    // metadata record instead of payload record.

[maniscalco, 2014/10/14 21:03:18]

Can you create crbug to add metadata records?

[pavely, 2014/10/16 22:13:29]

Done.

+    status = db_->Get(read_options, key, &data_str);
+    if (status.ok()) {
+      // Entry exists, don't overwrite.
+      continue;
+    } else if (!status.IsNotFound()) {
+      // Entry exists but failed to read.
+      DVLOG(1) << "DB::Get failed: status=" << status.ToString();
+      result_code = UNSPECIFIED_ERROR;
+      continue;
+    }
+    DCHECK(status.IsNotFound());
+
+    scoped_refptr<base::RefCountedMemory> data = iter->GetData();
+    leveldb::Slice data_slice(data->front_as<char>(), data->size());
+    status = db_->Put(write_options, key, data_slice);
+    if (!status.ok()) {
+      // Failed to write.
+      DVLOG(1) << "DB::Put failed: status=" << status.ToString();
+      result_code = UNSPECIFIED_ERROR;
+    }
+  }
+  callback_task_runner_->PostTask(FROM_HERE, base::Bind(callback, result_code));
+}
+
+void OnDiskAttachmentStore::Drop(const AttachmentIdList& ids,
+                                 const DropCallback& callback) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(db_);
+  Result result_code = SUCCESS;
+  leveldb::Status status;
+  leveldb::WriteOptions write_options;
+  write_options.sync = true;
+  AttachmentIdList::const_iterator iter = ids.begin();
+  AttachmentIdList::const_iterator end = ids.end();

[maniscalco, 2014/10/14 21:03:18]

Opportunity for const?

[pavely, 2014/10/16 22:13:29]

Done.

+  for (; iter != end; ++iter) {
+    std::string key;
+    FormatAttachmentDataKey(*iter, &key);
+    status = db_->Delete(write_options, key);

[maniscalco, 2014/10/14 21:03:18]

Declaring status outside the loop on purpose?  Would be good to be consistent
about in loop vs out of loop if all else is equal.

[pavely, 2014/10/16 22:13:29]

Done.

+    if (!status.ok()) {
+      // DB::Delete doesn't check if record exists, it returns ok just like
+      // AttachmentStore::Drop should.
+      DVLOG(1) << "DB::Delete failed: status=" << status.ToString();
+      result_code = UNSPECIFIED_ERROR;
+    }
+  }
+  callback_task_runner_->PostTask(FROM_HERE, base::Bind(callback, result_code));
+}
+
+void OnDiskAttachmentStore::FormatAttachmentDataKey(
+    const AttachmentId& attachment_id,
+    std::string* key) {
+  *key = kAttachmentDataStart;
+  attachment_id.GetProto().AppendToString(key);

[maniscalco, 2014/10/14 21:03:19]

So the DB key is "attachment_data1-<serialized attachment id proto>" ?

I guess I would have expected the key to contain the unique_id string and not
the proto itself.

[pavely, 2014/10/16 22:13:29]

Tradeoffs:
+ If I use unique_id, keys are smaller and more readable.
+ Using unique_id is more future safe. With proto in key any change to proto
will require database upgrade.
- when enumerating attachments I'll likely need to construct AttachmentIdProto,
this representation allows me to do this directly without storing
AttachmentIdProto separately in metadata record and without making significant
assumptions about future AttachmentIdProto content.

Done.

+}
+
+}  // namespace syncer