[SyncFS] Handle dirty tracker IDs in MetadataDatabaseIndexOnDisk

[SyncFS] Handle (demoted) dirty tracker IDs in MetadataDatabaseIndexOnDisk

BUG=347425
TEST=./unit_tests --gtest_filter="MetadataDatabaseInexOnDiskTest.*"

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

[peria, 2014-07-07 09:05:08]

PTL.

[nhiroki, 2014-07-07 09:52:29]

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc:387:
int64 tracker_id, leveldb::WriteBatch* batch) {
You may want to clarify that we don't have to use this |batch| here for
maintainability. For example,

void MetadataDatabaseIndex::DemoteDirtyTracker(
  int64 tracker_id, leveldb::WriteBatch* /* unused */) {

or

void MetadataDatabaseIndex::DemoteDirtyTracker(
    int64 tracker_id, leveldb::WriteBatch* batch) {
  // We don't have to add the tracker into |batch|.

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc:397:
leveldb::WriteBatch* batch) {
ditto.

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database_index.h
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index.h:99:
leveldb::WriteBatch* batch) OVERRIDE;
nit: can you make indents of arguments consistent with other function?

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_interface.h
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_interface.h:98:
leveldb::WriteBatch* batch) = 0;
ditto about indents.

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:309:
batch->Put(kDemotedDirtyIDKeyPrefix + base::Int64ToString(tracker_id), "");
GenerateDemotedDirtyIDKey()

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:338:
size_t MetadataDatabaseIndexOnDisk::CountDirtyTracker() const {
To avoid iterating, are we able to cache the number of dirty trackers?

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h:61:
leveldb::WriteBatch* batch) OVERRIDE;
ditto about ditto.

[tzik, 2014-07-07 14:28:47]

Looks good overall.

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1374:
WriteToDatabase(batch.Pass(), base::Bind(&EmptyStatusCallback));
I think consecutive call of this function will make the database inconsistent.
Since index class tries to read a data from the database before WriteToDatabase
completion.
Not necessary in this CL, but it's time to make WriteToDatabase() synchronous.

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:338:
size_t MetadataDatabaseIndexOnDisk::CountDirtyTracker() const {
On 2014/07/07 09:52:29, nhiroki wrote:
> To avoid iterating, are we able to cache the number of dirty trackers?

... Or just leave a comment for that.

[peria, 2014-07-08 03:05:08]

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1374:
WriteToDatabase(batch.Pass(), base::Bind(&EmptyStatusCallback));
On 2014/07/07 14:28:47, tzik wrote:
> I think consecutive call of this function will make the database inconsistent.
> Since index class tries to read a data from the database before
WriteToDatabase
> completion.
> Not necessary in this CL, but it's time to make WriteToDatabase() synchronous.

Add a TODO comment on WriteToDatabase().

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc:387:
int64 tracker_id, leveldb::WriteBatch* batch) {
On 2014/07/07 09:52:29, nhiroki wrote:
> You may want to clarify that we don't have to use this |batch| here for
> maintainability. For example,
> 
> void MetadataDatabaseIndex::DemoteDirtyTracker(
>   int64 tracker_id, leveldb::WriteBatch* /* unused */) {
> 
> or
> 
> void MetadataDatabaseIndex::DemoteDirtyTracker(
>     int64 tracker_id, leveldb::WriteBatch* batch) {
>   // We don't have to add the tracker into |batch|. 

Done.

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc:397:
leveldb::WriteBatch* batch) {
On 2014/07/07 09:52:29, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database_index.h
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index.h:99:
leveldb::WriteBatch* batch) OVERRIDE;
On 2014/07/07 09:52:29, nhiroki wrote:
> nit: can you make indents of arguments consistent with other function?

Done.

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_interface.h
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_interface.h:98:
leveldb::WriteBatch* batch) = 0;
On 2014/07/07 09:52:29, nhiroki wrote:
> ditto about indents.

Done.

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:309:
batch->Put(kDemotedDirtyIDKeyPrefix + base::Int64ToString(tracker_id), "");
On 2014/07/07 09:52:29, nhiroki wrote:
> GenerateDemotedDirtyIDKey()

Done.
Nice catch!

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:338:
size_t MetadataDatabaseIndexOnDisk::CountDirtyTracker() const {
On 2014/07/07 14:28:47, tzik wrote:
> On 2014/07/07 09:52:29, nhiroki wrote:
> > To avoid iterating, are we able to cache the number of dirty trackers?
> 
> ... Or just leave a comment for that.

Done.
Let me leave a TODO comment.

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h
(right):

https://codereview.chromium.org/377463002/diff/80011/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h:61:
leveldb::WriteBatch* batch) OVERRIDE;
On 2014/07/07 09:52:29, nhiroki wrote:
> ditto about ditto.

Done.

[nhiroki, 2014-07-08 04:17:21]

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc
(right):

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc:387:
int64 tracker_id, leveldb::WriteBatch* /*unused_batch*/) {
"/*unused_batch*/" -> "/* unused_batch */"

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc:397:
leveldb::WriteBatch* /*unused_batch*/) {
ditto.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc
(right):

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:282:
if (!StartsWithASCII(key, kDirtyIDKeyPrefix, true))
RemovePrefix() calls StartsWithASCII(), so we can (should) remove this
StartsWithASCII() (see the following URL).

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:326:
if (!StartsWithASCII(key, kDirtyIDKeyPrefix, true))
ditto.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:334:
batch->Put(kDemotedDirtyIDKeyPrefix + base::Int64ToString(tracker_id), "");
GenerateDemotedDirtyIDKey()

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:342:
//     everytime.
nit: You don't have to make an indent for comments.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:462:
DVLOG(1) << "  Add to dirty_trackers_: " << new_tracker.tracker_id();
"dirty_trackers_" does not exist in this scope, so can you update this log with
common words?

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:479:
DVLOG(1) << "  Remove from dirty_trackers_: " << tracker_id;
ditto.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:487:
DVLOG(1) << "  Add to dirty_trackers_: " << tracker_id;
ditto.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:502:
DVLOG(1) << "  Remove from dirty_trackers_: " << tracker_id;
ditto.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h
(right):

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h:72:
// Maintain indexes from AppIDs to tracker IDs.
nit: s/Maintain/Maintains/

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h:81:
// Maintains |dirty_trackers_| and |demoted_dirty_trackers_|.
This comment doesn't make sense for the on-memory index. Can you update?

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk_unittest.cc
(right):

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk_unittest.cc:300:
tracker = test_util::CreatePlaceholderTracker("dirty_holder_3",
This doesn't seem to be used.

[peria, 2014-07-08 04:58:33]

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc
(right):

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc:387:
int64 tracker_id, leveldb::WriteBatch* /*unused_batch*/) {
On 2014/07/08 04:17:20, nhiroki wrote:
> "/*unused_batch*/" -> "/* unused_batch */"

Done.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index.cc:397:
leveldb::WriteBatch* /*unused_batch*/) {
On 2014/07/08 04:17:20, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc
(right):

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:282:
if (!StartsWithASCII(key, kDirtyIDKeyPrefix, true))
On 2014/07/08 04:17:20, nhiroki wrote:
> RemovePrefix() calls StartsWithASCII(), so we can (should) remove this
> StartsWithASCII() (see the following URL).
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...

RemovePrefix() in this code is different from the one you suggested.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...

Though it also uses StartsWithASCII(), we should keep this StartsWithASCII() in
case of |key| starts with digits.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:326:
if (!StartsWithASCII(key, kDirtyIDKeyPrefix, true))
On 2014/07/08 04:17:20, nhiroki wrote:
> ditto.

ditto.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:334:
batch->Put(kDemotedDirtyIDKeyPrefix + base::Int64ToString(tracker_id), "");
On 2014/07/08 04:17:20, nhiroki wrote:
> GenerateDemotedDirtyIDKey()

Done.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:342:
//     everytime.
On 2014/07/08 04:17:20, nhiroki wrote:
> nit: You don't have to make an indent for comments.

Done.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:462:
DVLOG(1) << "  Add to dirty_trackers_: " << new_tracker.tracker_id();
On 2014/07/08 04:17:20, nhiroki wrote:
> "dirty_trackers_" does not exist in this scope, so can you update this log
with
> common words?

Done.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:479:
DVLOG(1) << "  Remove from dirty_trackers_: " << tracker_id;
On 2014/07/08 04:17:20, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:487:
DVLOG(1) << "  Add to dirty_trackers_: " << tracker_id;
On 2014/07/08 04:17:20, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:502:
DVLOG(1) << "  Remove from dirty_trackers_: " << tracker_id;
On 2014/07/08 04:17:20, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h
(right):

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h:72:
// Maintain indexes from AppIDs to tracker IDs.
On 2014/07/08 04:17:20, nhiroki wrote:
> nit: s/Maintain/Maintains/

The (hidden) subject of this sentence is "Following methods",
so I think it better to keep it without 's'.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h:81:
// Maintains |dirty_trackers_| and |demoted_dirty_trackers_|.
On 2014/07/08 04:17:20, nhiroki wrote:
> This comment doesn't make sense for the on-memory index. Can you update?

Done.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk_unittest.cc
(right):

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk_unittest.cc:300:
tracker = test_util::CreatePlaceholderTracker("dirty_holder_3",
On 2014/07/08 04:17:20, nhiroki wrote:
> This doesn't seem to be used.

Done.

[nhiroki, 2014-07-08 05:20:10]

LGTM. Please wait for tzik's review.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc
(right):

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:282:
if (!StartsWithASCII(key, kDirtyIDKeyPrefix, true))
On 2014/07/08 04:58:32, peria wrote:
> On 2014/07/08 04:17:20, nhiroki wrote:
> > RemovePrefix() calls StartsWithASCII(), so we can (should) remove this
> > StartsWithASCII() (see the following URL).
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
> 
> RemovePrefix() in this code is different from the one you suggested.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...
> 
> Though it also uses StartsWithASCII(), we should keep this StartsWithASCII()
in
> case of |key| starts with digits.

Sorry for my unclear comment. I mean you can modify this RemovePrefix() like
ServiceWorker's one to avoid calling StartsWithASCII() twice. This is optional
and a separate patch should be OK.

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h
(right):

https://codereview.chromium.org/377463002/diff/140001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.h:81:
// Maintains |dirty_trackers_| and |demoted_dirty_trackers_|.
On 2014/07/08 04:58:33, peria wrote:
> On 2014/07/08 04:17:20, nhiroki wrote:
> > This comment doesn't make sense for the on-memory index. Can you update?
> 
> Done.

Ah, sorry... "for the on-disk index" is correct :p

[tzik, 2014-07-08 07:34:59]

lgtm

[peria, 2014-07-08 07:37:54]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-07-08 07:38:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peria@chromium.org/377463002/180001

[commit-bot: I haz the power, 2014-07-08 11:30:51]

Change committed as 281720