Deprecate DriveMetadataStore.batch_sync_origins

Deprecate DriveMetadataStore.batch_sync_origins

Deprecate batch_sync_origins from DriveMetadataStore so batch
origins are no longer duplicated in both
DriveFileSyncService.pending_batch_sync_origins and
DriveMetadataStore.batch_sync_origins_;

BUG=229764
TEST=DriveMetadataStoreTest.DeprecateBatchSyncOrigins
+ Existing unit_tests --gtest_filter=DriveFileSync*
+ Existing unit_tests --gtest_filter=DriveMeta*

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

[calvinlo, 2013-05-19 12:48:51]

Hi tzik san, I think I've managed to remove batch_sync_origins from
DriveMetadataStore completely as I believe you wanted.

This patch is mostly complete but there's one test which is failing on the last
case because of a problem with the Task token and I'm not sure why. Can you
please speak to me tomorrow when you are free about this. I can't tell right now
if it's a bug in my code or the TaskManager.

I also wanted to double check with you on how you want to handle the DB
migration that will result from this change. I'm thinking we can do something
like add in an if check to see if there's any LevelDB keys with the batch sync
prefix left, and if there are, to delete them.

Thank you.

[calvinlo, 2013-05-25 07:55:36]

Tzik san, I think this is ready to be looked at after rebase and some test
revisions. It's still running on the trybots but unit_tests.Drive* tests pass
locally. Please review again at your convenience.

[tzik, 2013-05-27 04:31:58]

Looks good.

https://codereview.chromium.org/15410005/diff/15001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store_unittest.cc (right):

https://codereview.chromium.org/15410005/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:674:
leveldb::Status status = leveldb::DB::Open(options, db_dir, &db_ptr);
We can open only one LevelDB instance for each path, that seems to cause the
Windows test failure.

Can we get it from metadata_store()?
Or, just
 - Close the DB before InitializeDatabase(),
 - DropDatabase(),
 - Reopen the DB,
may work.

[tzik, 2013-05-27 07:57:42]

https://codereview.chromium.org/15410005/diff/15001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store_unittest.cc (right):

https://codereview.chromium.org/15410005/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:674:
leveldb::Status status = leveldb::DB::Open(options, db_dir, &db_ptr);
On 2013/05/27 04:31:58, tzik wrote:
> We can open only one LevelDB instance for each path, that seems to cause the
> Windows test failure.
> 
> Can we get it from metadata_store()?
> Or, just
>  - Close the DB before InitializeDatabase(),
>  - DropDatabase(),
>  - Reopen the DB,
> may work.

For second option, we need to ensure the LevelDB instance was dropped before the
reopen. Since it is closed asynchronously on FILE thread.

Please insert something like below (it may need a helper function)
file_task_runner_->PostTaskAndReply(
    FROM_HERE,
    base::Bind(&Noop),
    base::Bind(&FunctionToCallMessageLoopQuit));
message_loop_->Run();

[calvinlo, 2013-05-27 09:10:34]

On 2013/05/27 07:57:42, tzik wrote:
>
https://codereview.chromium.org/15410005/diff/15001/chrome/browser/sync_file_...
> File chrome/browser/sync_file_system/drive_metadata_store_unittest.cc (right):
> 
>
https://codereview.chromium.org/15410005/diff/15001/chrome/browser/sync_file_...
> chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:674:
> leveldb::Status status = leveldb::DB::Open(options, db_dir, &db_ptr);
> On 2013/05/27 04:31:58, tzik wrote:
> > We can open only one LevelDB instance for each path, that seems to cause the
> > Windows test failure.
> > 
> > Can we get it from metadata_store()?
> > Or, just
> >  - Close the DB before InitializeDatabase(),
> >  - DropDatabase(),
> >  - Reopen the DB,
> > may work.
> 
> For second option, we need to ensure the LevelDB instance was dropped before
the
> reopen. Since it is closed asynchronously on FILE thread.
> 
> Please insert something like below (it may need a helper function)
> file_task_runner_->PostTaskAndReply(
>     FROM_HERE,
>     base::Bind(&Noop),
>     base::Bind(&FunctionToCallMessageLoopQuit));
> message_loop_->Run();

Taiju, per chat, I have taken a hybrid solution from your two suggestions.
However, after looking at it more, I'm wondering if the cleaner soluton may be
instead to leave a legacy function inside MetadataStoreDB that returns a bool
saying whether any batch sync origins are left.

[tzik, 2013-05-27 09:23:54]

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:378: return
db_.get()->db_.get();
former get() can be omitted?

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:774: for
(batch_origin_itr->SeekToFirst();
Seek(kDriveBatchSyncOriginKeyPrefix)?

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:778: if
(key.find(kDriveBatchSyncOriginKeyPrefix) == std::string::npos)
!StartsWithASCII?

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:779: continue;
s/continue/break/?
The contents are sorted by the key.

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.h (right):

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.h:171: leveldb::DB*
level_db();
Can you rename this with ForTesting like GetDBInstanceForTesting?

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store_unittest.cc (right):

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:246:
leveldb::DB* metadata_db() {
metadata_db() looks confusing on this context.
level_db() or backing_level_db()?

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:247: return
drive_metadata_store_.get()->level_db();
maybe, get() can be omitted.

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:676:
base_dir().Append(DriveMetadataStore::kDatabaseName));
Please move them into the block below.

[calvinlo, 2013-05-27 09:47:47]

tzik san, PTAL again. If you're happy with this approach then I'll leave the
test as is. the win TryBot is running just to make sure it does indeed work on
windows now.

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:378: return
db_.get()->db_.get();
On 2013/05/27 09:23:54, tzik wrote:
> former get() can be omitted?

Done.

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:774: for
(batch_origin_itr->SeekToFirst();
On 2013/05/27 09:23:54, tzik wrote:
> Seek(kDriveBatchSyncOriginKeyPrefix)?

Done.

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:779: continue;
On 2013/05/27 09:23:54, tzik wrote:
> s/continue/break/?
> The contents are sorted by the key.

Done.

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.h (right):

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.h:171: leveldb::DB*
level_db();
On 2013/05/27 09:23:54, tzik wrote:
> Can you rename this with ForTesting like GetDBInstanceForTesting?

Done.

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store_unittest.cc (right):

https://codereview.chromium.org/15410005/diff/32001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:676:
base_dir().Append(DriveMetadataStore::kDatabaseName));
On 2013/05/27 09:23:54, tzik wrote:
> Please move them into the block below.

Done. Sorry, forget to do this after I decided to do the proxying through
metadata_store() to get the leveldb::DB pointer again in the verification part.

[tzik, 2013-05-27 12:14:10]

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:778: if
(!StartsWithASCII(key, kDriveBatchSyncOriginKeyPrefix, false))
s/false/true/
This should be case sensitive.

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store_unittest.cc (right):

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:668: const
std::string kSyncRootResourceId("sync_root_resource_id");
unused?

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:670: const
std::string kFileResourceId("piyo");
unused?

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:684:
db.reset();
scoped_ptr deletes its content on scope out.
so reset() is not needed.

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:693: for
(itr->SeekToFirst(); itr->Valid(); itr->Next()) {
Seek(kDriveBatchSyncOriginKeyPrefix)?

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:695: if
(key.find(kDriveBatchSyncOriginKeyPrefix) != std::string::npos)
StartsWithASCII and break? as we do at drive_metadata.cc:778

[calvinlo, 2013-05-28 02:18:55]

PTAL again.

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:778: if
(!StartsWithASCII(key, kDriveBatchSyncOriginKeyPrefix, false))
On 2013/05/27 12:14:10, tzik wrote:
> s/false/true/
> This should be case sensitive.

Done.

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store_unittest.cc (right):

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:668: const
std::string kSyncRootResourceId("sync_root_resource_id");
On 2013/05/27 12:14:10, tzik wrote:
> unused?

Done.

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:670: const
std::string kFileResourceId("piyo");
On 2013/05/27 12:14:10, tzik wrote:
> unused?

Done.

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:684:
db.reset();
On 2013/05/27 12:14:10, tzik wrote:
> scoped_ptr deletes its content on scope out.
> so reset() is not needed.

Done.

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:693: for
(itr->SeekToFirst(); itr->Valid(); itr->Next()) {
On 2013/05/27 12:14:10, tzik wrote:
> Seek(kDriveBatchSyncOriginKeyPrefix)?

Done.

https://codereview.chromium.org/15410005/diff/34002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:695: if
(key.find(kDriveBatchSyncOriginKeyPrefix) != std::string::npos)
On 2013/05/27 12:14:10, tzik wrote:
> StartsWithASCII and break? as we do at drive_metadata.cc:778

Done.

[tzik, 2013-05-28 02:23:21]

lgtm

[commit-bot: I haz the power, 2013-05-28 02:31:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/15410005/49001

[commit-bot: I haz the power, 2013-05-28 05:03:58]

Change committed as 202502