Add per attachment metadata records.

Add per attachment metadata records.

Currently metadata record only contains size and it is not used anywhere.
In the future size will be used for storage management by datatypes.

Next step is to add crc to metadata record.

BUG=424304
R=maniscalco@chromium.org

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

[pavely, 2014-10-29 20:11:20]

PTAL.

[maniscalco, 2014-10-30 16:40:20]

Just a few nits:

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/on_disk_attachment_store.cc:49:
read_options.verify_checksums = false;
WDYT about enabling leveldb checksums?  Here's my thinking...

The performance cost is likely low relatively to the operation code (disk io and
memcpys).

I would expect that leveldb handles invalid checksums appropriately (it's a
database; that's part of its job).

Catching invalid checksums when reading/writing to/from leveldb may reduce the
scope that we have to examine when we encounter corruption in the wild.

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/proto/attachment_store.proto (right):

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/proto/attachment_store.proto:21: // record from
actual data allows to enumerate attachments in the store without
allows to -> allows us to (and again later in this paragraph)

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/proto/attachment_store.proto:24: message
AttachmentRecordMetadata {
I remember that we already have a proto called AttachmentMetadataRecord (stored
in sync entities and Directory).  Naming is hard.

Are there other good names we could use?  Some ideas:

1. Drop Attachment from the message names (StoreMetadata, RecordMetadata).

2. Record -> Entry?

3. ?

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/proto/attachment_store.proto:26: optional int64
attachment_size = 2;
2 not 1?

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/public/att...
File sync/internal_api/public/attachments/on_disk_attachment_store.h (right):

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/on_disk_attachment_store.h:50: friend class
OnDiskAttachmentStoreSpecificTest;
Would FRIEND_TEST_ALL_PREFIXES work here?

[pavely, 2014-10-30 17:14:53]

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

[pavely, 2014-10-30 17:14:53]

pavely@chromium.org changed required reviewers:
+ maniscalco@chromium.org

[pavely, 2014-10-30 17:14:53]

Adding Stan per his request. Forgot to do it yesterday.

[pavely, 2014-10-30 18:28:37]

PTAL

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/on_disk_attachment_store.cc:49:
read_options.verify_checksums = false;
On 2014/10/30 16:40:20, maniscalco wrote:
> WDYT about enabling leveldb checksums?  Here's my thinking...
> 
> The performance cost is likely low relatively to the operation code (disk io
and
> memcpys).
> 
> I would expect that leveldb handles invalid checksums appropriately (it's a
> database; that's part of its job).
> 
> Catching invalid checksums when reading/writing to/from leveldb may reduce the
> scope that we have to examine when we encounter corruption in the wild.

I agree, letting leveldb can provide small additional benefit of differentiating
case when data rotted on disk vs. data never made it to disk intact.

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/proto/attachment_store.proto (right):

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/proto/attachment_store.proto:21: // record from
actual data allows to enumerate attachments in the store without
On 2014/10/30 16:40:20, maniscalco wrote:
> allows to -> allows us to (and again later in this paragraph)

Done.

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/proto/attachment_store.proto:24: message
AttachmentRecordMetadata {
On 2014/10/30 16:40:20, maniscalco wrote:
> I remember that we already have a proto called AttachmentMetadataRecord
(stored
> in sync entities and Directory).  Naming is hard.
> 
> Are there other good names we could use?  Some ideas:
> 
> 1. Drop Attachment from the message names (StoreMetadata, RecordMetadata).
> 
> 2. Record -> Entry?
> 
> 3. ?

This proto file already declares attachment_store_pb namespace. I'm dropping
Metadata from both names.

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/proto/attachment_store.proto:26: optional int64
attachment_size = 2;
On 2014/10/30 16:40:20, maniscalco wrote:
> 2 not 1?

Sorry, I started with CRC, but reverted it.

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/public/att...
File sync/internal_api/public/attachments/on_disk_attachment_store.h (right):

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/on_disk_attachment_store.h:50: friend class
OnDiskAttachmentStoreSpecificTest;
On 2014/10/30 16:40:20, maniscalco wrote:
> Would FRIEND_TEST_ALL_PREFIXES work here? 
FRIEND_TEST_ALL_PREFIXES is for befriending individual tests:
  FRIEND_TEST_ALL_PREFIXES(OnDiskAttachmentStoreSpecificTest,
AttachmentRecordMetadata)

In this case I put helper function into test case class and want it to have
access to OnDiskAttachmentStore.

[maniscalco, 2014-10-30 18:33:10]

LGTM

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/proto/attachment_store.proto (right):

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/proto/attachment_store.proto:24: message
AttachmentRecordMetadata {
On 2014/10/30 18:28:36, pavely wrote:
> On 2014/10/30 16:40:20, maniscalco wrote:
> > I remember that we already have a proto called AttachmentMetadataRecord
> (stored
> > in sync entities and Directory).  Naming is hard.
> > 
> > Are there other good names we could use?  Some ideas:
> > 
> > 1. Drop Attachment from the message names (StoreMetadata, RecordMetadata).
> > 
> > 2. Record -> Entry?
> > 
> > 3. ?
> 
> This proto file already declares attachment_store_pb namespace. I'm dropping
> Metadata from both names.

SGTM

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/public/att...
File sync/internal_api/public/attachments/on_disk_attachment_store.h (right):

https://codereview.chromium.org/690723004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/on_disk_attachment_store.h:50: friend class
OnDiskAttachmentStoreSpecificTest;
On 2014/10/30 18:28:36, pavely wrote:
> On 2014/10/30 16:40:20, maniscalco wrote:
> > Would FRIEND_TEST_ALL_PREFIXES work here? 
> FRIEND_TEST_ALL_PREFIXES is for befriending individual tests:
>   FRIEND_TEST_ALL_PREFIXES(OnDiskAttachmentStoreSpecificTest,
> AttachmentRecordMetadata)
> 
> In this case I put helper function into test case class and want it to have
> access to OnDiskAttachmentStore.

I see.

[stanisc, 2014-10-30 18:56:59]

lgtm

https://codereview.chromium.org/690723004/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/690723004/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:28: const char
kMetadataPrefix[] = "metadata-";
It probably doesn't matter in this case but I'd mention it anyway in case we use
this store for anything with higher volume of operations than attachments.
LevelDB physically organizes the data according to the key sorting order. If
this code always reads/writes metadata next to data, then it would make sense
for the keys to be next to each other. Using a suffix might work better than
using a prefix.

https://codereview.chromium.org/690723004/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/proto/attachment_store.proto (right):

https://codereview.chromium.org/690723004/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/proto/attachment_store.proto:20: // Metadata for
attachment in attachemnt store. Storing metadata in separate
Nit: typo - attachemnt

[pavely, 2014-10-30 19:53:49]

The CQ bit was checked by pavely@chromium.org

[pavely, 2014-10-30 19:54:15]

https://codereview.chromium.org/690723004/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/690723004/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:28: const char
kMetadataPrefix[] = "metadata-";
On 2014/10/30 18:56:59, stanisc wrote:
> It probably doesn't matter in this case but I'd mention it anyway in case we
use
> this store for anything with higher volume of operations than attachments.
> LevelDB physically organizes the data according to the key sorting order. If
> this code always reads/writes metadata next to data, then it would make sense
> for the keys to be next to each other. Using a suffix might work better than
> using a prefix.

One scenario that we will likely need to do is scan attachment metadatas after
restart. I wanted to avoid having to read or skip over attachment data records
in this case, that would be expensive and wasteful. Also attachment metadatas
are read with caching so after initial read they are served from cache.

https://codereview.chromium.org/690723004/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/proto/attachment_store.proto (right):

https://codereview.chromium.org/690723004/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/proto/attachment_store.proto:20: // Metadata for
attachment in attachemnt store. Storing metadata in separate
On 2014/10/30 18:56:59, stanisc wrote:
> Nit: typo - attachemnt

Done.

[commit-bot: I haz the power, 2014-10-30 19:54:52]

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

[commit-bot: I haz the power, 2014-10-30 21:59:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-30 21:59:36]

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