[SyncFS] Retry initialization of MetadataDatabase on failure of loading ServiceMetadata

[SyncFS] Retry initialization of MetadataDatabase
if it fails to load ServiceMetadata's information from DB.
It tries to initialize for 5 times.


BUG=409737
TEST=./unit_tests --gtest_filter="Metadata*

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

[peria, 2014-09-02 06:42:30]

peria@chromium.org changed reviewers:
+ nhiroki@chromium.org, tzik@chromium.org

[peria, 2014-09-02 06:42:31]

PTL

[nhiroki, 2014-09-02 07:26:04]

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

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1469:
db_->Commit();
If you want to delete all records, can you use leveldb::DestroyDB() to avoid a
full scan?

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1471: return
SYNC_DATABASE_ERROR_FAILED;
Let me confirm... this makes the SyncFS disabled until reloading the app. Is
this right?

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:646:
TEST_F(MetadataDatabaseTest, InitializationTest_Corruption) {
I think this and other tests in MetadataDatabaseTest don't work for the on-disk
indexing version since it's disabled by the flag. How about enable them for both
versions? (a separate patch is OK)

[peria, 2014-09-02 07:36:27]

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

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1469:
db_->Commit();
On 2014/09/02 07:26:04, nhiroki wrote:
> If you want to delete all records, can you use leveldb::DestroyDB() to avoid a
> full scan?

We have to update LevelDBWrapper class to use it :(
Will do in another patch.

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1471: return
SYNC_DATABASE_ERROR_FAILED;
On 2014/09/02 07:26:04, nhiroki wrote:
> Let me confirm... this makes the SyncFS disabled until reloading the app. Is
> this right?

Yes, it is.
We have to run SyncEngineInitializer again.

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:646:
TEST_F(MetadataDatabaseTest, InitializationTest_Corruption) {
On 2014/09/02 07:26:04, nhiroki wrote:
> I think this and other tests in MetadataDatabaseTest don't work for the
on-disk
> indexing version since it's disabled by the flag. How about enable them for
both
> versions? (a separate patch is OK)

filed http://crbug.com/409743

[nhiroki, 2014-09-02 08:36:25]

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

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1468:
itr->Delete();
Although not related to this CL and you don't have to address now, how about
renaming 'Delete()' to 'DeleteAndAdvance()' or something else? At first I was a
bit confused at this iteration because nobody calls itr->Next()...

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1469:
db_->Commit();
On 2014/09/02 07:36:27, peria wrote:
> On 2014/09/02 07:26:04, nhiroki wrote:
> > If you want to delete all records, can you use leveldb::DestroyDB() to avoid
a
> > full scan?
> 
> We have to update LevelDBWrapper class to use it :(
> Will do in another patch.

Okay, can we have a TODO comment about that?

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1471: return
SYNC_DATABASE_ERROR_FAILED;
On 2014/09/02 07:36:27, peria wrote:
> On 2014/09/02 07:26:04, nhiroki wrote:
> > Let me confirm... this makes the SyncFS disabled until reloading the app. Is
> > this right?
> 
> Yes, it is.
> We have to run SyncEngineInitializer again.

I see... there would be another option to retry the initialization from here
(either is ok to me).

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:646:
TEST_F(MetadataDatabaseTest, InitializationTest_Corruption) {
On 2014/09/02 07:36:27, peria wrote:
> On 2014/09/02 07:26:04, nhiroki wrote:
> > I think this and other tests in MetadataDatabaseTest don't work for the
> on-disk
> > indexing version since it's disabled by the flag. How about enable them for
> both
> > versions? (a separate patch is OK)
> 
> filed http://crbug.com/409743

Acknowledged.

[peria, 2014-09-03 04:08:14]

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

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1468:
itr->Delete();
On 2014/09/02 08:36:25, nhiroki wrote:
> Although not related to this CL and you don't have to address now, how about
> renaming 'Delete()' to 'DeleteAndAdvance()' or something else? At first I was
a
> bit confused at this iteration because nobody calls itr->Next()...

Acknowledged.

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1469:
db_->Commit();
On 2014/09/02 08:36:25, nhiroki wrote:
> On 2014/09/02 07:36:27, peria wrote:
> > On 2014/09/02 07:26:04, nhiroki wrote:
> > > If you want to delete all records, can you use leveldb::DestroyDB() to
avoid
> a
> > > full scan?
> > 
> > We have to update LevelDBWrapper class to use it :(
> > Will do in another patch.
> 
> Okay, can we have a TODO comment about that?

Done.

https://codereview.chromium.org/527163002/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1471: return
SYNC_DATABASE_ERROR_FAILED;
On 2014/09/02 08:36:25, nhiroki wrote:
> On 2014/09/02 07:36:27, peria wrote:
> > On 2014/09/02 07:26:04, nhiroki wrote:
> > > Let me confirm... this makes the SyncFS disabled until reloading the app.
Is
> > > this right?
> > 
> > Yes, it is.
> > We have to run SyncEngineInitializer again.
> 
> I see... there would be another option to retry the initialization from here
> (either is ok to me).

Acknowledged.

[peria, 2014-09-03 04:11:13]

I changed the description and the core routine as the description shows.
PTAL.

[nhiroki, 2014-09-03 04:30:06]

lgtm

https://codereview.chromium.org/527163002/diff/60001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/527163002/diff/60001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:643:
EXPECT_EQ(SYNC_STATUS_OK, InitializeMetadataDatabase());
This init->drop-init sequence might not make sense when using
|in_memory_env_|...?

[tzik, 2014-09-03 04:47:37]

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

https://codereview.chromium.org/527163002/diff/60001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1430: for
(int i = 0; i < kMaxRetry && status != SYNC_STATUS_OK; ++i) {
I think 2 is enough for the number of retry.

Can you add metadata_database.reset() here to avoid creating multiple instance
of leveldb::DB?

[peria, 2014-09-03 09:24:21]

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

https://codereview.chromium.org/527163002/diff/60001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1430: for
(int i = 0; i < kMaxRetry && status != SYNC_STATUS_OK; ++i) {
On 2014/09/03 04:47:37, tzik wrote:
> I think 2 is enough for the number of retry.
> 
> Can you add metadata_database.reset() here to avoid creating multiple instance
> of leveldb::DB?

Done.

https://codereview.chromium.org/527163002/diff/60001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/527163002/diff/60001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:643:
EXPECT_EQ(SYNC_STATUS_OK, InitializeMetadataDatabase());
It works well as long as we use |in_memory_env_|.

ref. 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/leveld...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/leveld...

[nhiroki, 2014-09-03 09:52:20]

https://codereview.chromium.org/527163002/diff/60001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/527163002/diff/60001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:643:
EXPECT_EQ(SYNC_STATUS_OK, InitializeMetadataDatabase());
On 2014/09/03 09:24:21, peria wrote:
> It works well as long as we use |in_memory_env_|.
> 
> ref. 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/leveld...
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/leveld...


I see. I wrongly assumed |in_memory_env_| is recreated in the sequence. Thanks
for your explanation.

[tzik, 2014-09-03 12:54:15]

lgtm

[peria, 2014-09-04 02:51:01]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-09-04 02:53:31]

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

[commit-bot: I haz the power, 2014-09-04 03:56:38]

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

[commit-bot: I haz the power, 2014-09-04 03:56:39]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[peria, 2014-09-04 05:06:36]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-09-04 05:07:01]

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

[commit-bot: I haz the power, 2014-09-04 06:08:12]

Committed patchset #3 (id:80001) as 0126a594a17d2251e610ed0da2338e0d88f861ae

[commit-bot: I haz the power, 2014-09-10 03:30:17]

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