Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15023022:
Add to be fetched files instead of DriveMetadataStore.batch_sync_origins_ (Closed)
|
Created:
7 years, 7 months ago by calvinlo Modified:
7 years, 7 months ago CC:
chromium-reviews, tzik+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd to be fetched files instead of DriveMetadataStore.batch_sync_origins_
This persists files to be fetched to the DB so they can be
restored in case Chrome crashes. The origin itself is no
longer saved but it does get added back to
pending_batch_sync_origins anyway when the SyncFS app is
loaded again.
Pre-requisite to deprecating batch_sync_origins from
DriveMetadataStore
BUG=229764
TEST=Existing unit_tests --gtest_filter=DriveFileSyncService*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202025
Patch Set 1 #Patch Set 2 : Updated Tests #
Total comments: 4
Patch Set 3 : tzik review #1 #
Total comments: 2
Patch Set 4 : tzik review #2 #
Total comments: 8
Patch Set 5 : tzik review #3 #
Total comments: 11
Patch Set 6 : UpdateRegisteredOrigins tests is currently broken because of TaskToken not being released on shutdo… #Patch Set 7 : tzik review #4 #
Total comments: 18
Patch Set 8 : Update to ensure DriveMetadataStore.batch_sync_origins is not used. #Patch Set 9 : fixup after manual diff #
Total comments: 2
Patch Set 10 : Sorry made a typo after manual diff check fixups. Fix to compile. #Patch Set 11 : Added TODO about batch_sync_origin deprecation from DB. #
Total comments: 4
Patch Set 12 : tzik review, test revisions #Messages
Total messages: 30 (0 generated)
tzik san, please PTAL. As we discussed offline, I will do the actual remove of the batch_sync_origin variable and related functions in the next patch to keep this patch clean.
https://codereview.chromium.org/15023022/diff/2001/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/2001/chrome/browser/sync_file_s... chrome/browser/sync_file_system/drive_file_sync_service.cc:750: metadata_store_->AddBatchSyncOrigin(origin, resource_id); could you move this around line 845? I want register the origin as a incremental sync origin from first. https://codereview.chromium.org/15023022/diff/2001/chrome/browser/sync_file_s... chrome/browser/sync_file_system/drive_file_sync_service.cc:848: metadata_store_->MoveBatchSyncOriginToIncremental(origin); could you do this unconditionally?
https://codereview.chromium.org/15023022/diff/2001/chrome/browser/sync_file_s... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/2001/chrome/browser/sync_file_s... chrome/browser/sync_file_system/drive_file_sync_service.cc:750: metadata_store_->AddBatchSyncOrigin(origin, resource_id); On 2013/05/16 12:11:34, tzik wrote: > could you move this around line 845? > I want register the origin as a incremental sync origin from first. Done. As discussed, in order to facilitate this change I have updated pending_batch_sync_origins_ to be a map so it can also hold the resource_id. https://codereview.chromium.org/15023022/diff/2001/chrome/browser/sync_file_s... chrome/browser/sync_file_system/drive_file_sync_service.cc:848: metadata_store_->MoveBatchSyncOriginToIncremental(origin); On 2013/05/16 12:11:34, tzik wrote: > could you do this unconditionally? Done.
https://codereview.chromium.org/15023022/diff/15001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/15001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1956: return; This part changes the behavior: new code doesn't return if |pending_batch_sync_origins| is non-empty. Is it intentional?
Also fixed the broken tests https://codereview.chromium.org/15023022/diff/15001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/15001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1956: return; On 2013/05/17 05:29:36, tzik wrote: > This part changes the behavior: new code doesn't return if > |pending_batch_sync_origins| is non-empty. Is it intentional? Thanks for catching this. Sorry, I didn't notice that. I'll revert this back to the way it was before.
Looks good! https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:554: origin, metadata_store_->disabled_origins().find(origin)->second)); can this be disabled_origins()[origin] for better readability? maybe, this can be just like this: pending_batch_sync_origins_.insert(*metadata_store_->disabled_origins().find(origin)); https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:700: pending_batch_sync_origins_.insert(std::make_pair(origin, resource_id)); ditto https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:711: std::string resource_id = pending_batch_sync_origins_.find(origin)->second; can be pending_batch_sync_origins_.begin()->second? https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_metadata_store.cc (right): https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:543: if (IsBatchSyncOrigin(origin)) can this case happen?
https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:554: origin, metadata_store_->disabled_origins().find(origin)->second)); On 2013/05/17 09:36:56, tzik wrote: > can this be disabled_origins()[origin] for better readability? > > maybe, this can be just like this: > pending_batch_sync_origins_.insert(*metadata_store_->disabled_origins().find(origin)); Done. Sorry, I forgot that the map types match so I can just call find directly. https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:700: pending_batch_sync_origins_.insert(std::make_pair(origin, resource_id)); On 2013/05/17 09:36:56, tzik wrote: > ditto Done. https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:711: std::string resource_id = pending_batch_sync_origins_.find(origin)->second; On 2013/05/17 09:36:56, tzik wrote: > can be pending_batch_sync_origins_.begin()->second? Done. https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_metadata_store.cc (right): https://codereview.chromium.org/15023022/diff/24001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:543: if (IsBatchSyncOrigin(origin)) On 2013/05/17 09:36:56, tzik wrote: > can this case happen? Yes, in some of the tests a batch sync origin is added. The message loop then runs and the new code adds the origin to batch again before moving it to incremental. This happens because of the line you asked me to move in the first review . https://codereview.chromium.org/15023022/diff/15001/chrome/browser/sync_file_... Line 762. This used to be protected with a condition if (!metadata_store_->IsKnownOrigin(origin)). Either I can add that condition to line drive_file_sync_service 852, or I can do the check here. I'm not sure it actually matters though as I'm going to kill off the batch sync next anyway.
nit: Can you try to make the issue description git friendly: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/sRRWJwvy2C8/qIWO7... Style for detailed description part is not strictly followed by everyone (someone say it must be wrapped around col=72 while others say col=76) but it's nice to have a capital short summary line (followed by one empty line) at the top. https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (left): https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:445: } Drive-by question (probaby to tzik): when are we adding existing batch sync origins (that are not yet migrated in drive_metadata) to pending_batch_sync_origins? (Just want to make sure) It'be nice how batch sync origin changes before and after this patch.
On 2013/05/20 02:42:08, kinuko wrote: > nit: Can you try to make the issue description git friendly: > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/sRRWJwvy2C8/qIWO7... > > Style for detailed description part is not strictly followed by everyone > (someone say it must be wrapped around col=72 while others say col=76) but it's > nice to have a capital short summary line (followed by one empty line) at the > top. > > https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... > File chrome/browser/sync_file_system/drive_file_sync_service.cc (left): > > https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... > chrome/browser/sync_file_system/drive_file_sync_service.cc:445: } > Drive-by question (probaby to tzik): when are we adding existing batch sync > origins (that are not yet migrated in drive_metadata) to > pending_batch_sync_origins? (Just want to make sure) > > It'be nice how batch sync origin changes before and after this patch. Update summary and description.
On 2013/05/20 02:42:08, kinuko wrote: > nit: Can you try to make the issue description git friendly: > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/sRRWJwvy2C8/qIWO7... > > Style for detailed description part is not strictly followed by everyone > (someone say it must be wrapped around col=72 while others say col=76) but it's > nice to have a capital short summary line (followed by one empty line) at the > top. > > https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... > File chrome/browser/sync_file_system/drive_file_sync_service.cc (left): > > https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... > chrome/browser/sync_file_system/drive_file_sync_service.cc:445: } > Drive-by question (probaby to tzik): when are we adding existing batch sync > origins (that are not yet migrated in drive_metadata) to > pending_batch_sync_origins? (Just want to make sure) > > It'be nice how batch sync origin changes before and after this patch. Updated summary and description.
Thx for updating the description. I think I still have a question reg: when we populate pending_batch_sync_origins for existing ones. https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:2194: pending_batch_sync_origins_.insert(std::make_pair(origin, resource_id)); To me it looks we only (re-)add existing batch sync origins to pending_batch_sync_origins when GetResourceIdForOrigin() returns an empty resource_id for the origin when the app is reloaded? I could be wrong.
https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (left): https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:445: } On 2013/05/20 02:42:08, kinuko wrote: > Drive-by question (probaby to tzik): when are we adding existing batch sync > origins (that are not yet migrated in drive_metadata) to > pending_batch_sync_origins? (Just want to make sure) > > It'be nice how batch sync origin changes before and after this patch. I'm not sure I understand your question. If you're referring to why removing this code block doesn't break the current behavior than I believe that I already put the answer in the issue description. Instead of restoring whole origins back into pending_batch_sync_origins_ the files belonging to that origin have their to_be_fetched flag set as true. They are then restored in lines 447 - 460. The files have their to_be_fetch bit set in DidGetDirectoryContentForBatchSync. In the case that Chrome crashes before the files for the origin are added as to be fetched, they will only be picked up again when the extension is reloaded and the origin is added as if it was new. Does this answer your question?
https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (left): https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:445: } On 2013/05/20 03:53:14, calvinlo wrote: > On 2013/05/20 02:42:08, kinuko wrote: > > Drive-by question (probaby to tzik): when are we adding existing batch sync > > origins (that are not yet migrated in drive_metadata) to > > pending_batch_sync_origins? (Just want to make sure) > > > > It'be nice how batch sync origin changes before and after this patch. > > I'm not sure I understand your question. If you're referring to why removing > this code block doesn't break the current behavior than I believe that I already > put the answer in the issue description. > > Instead of restoring whole origins back into pending_batch_sync_origins_ the > files belonging to that origin have their to_be_fetched flag set as true. They > are then restored in lines 447 - 460. The files have their to_be_fetch bit set > in DidGetDirectoryContentForBatchSync. > > In the case that Chrome crashes before the files for the origin are added as to > be fetched, they will only be picked up again when the extension is reloaded and > the origin is added as if it was new. I think I'm asking about the latter case, and my previous comment (for line 2194) was about my concern that it doesn't look it's always added back at a quick glance. We may add back the origin at line 753 or at line 2194, but line 752 will return false if the origin is already in batch-sync-origin list and we only go through line 2194 if the origin's resource_id has not been fetched yet (but it's usually not true for already-registered batch sync origins)? I'm happy to be corrected. > Does this answer your question?
https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (left): https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:445: } On 2013/05/20 04:06:22, kinuko wrote: > On 2013/05/20 03:53:14, calvinlo wrote: > > On 2013/05/20 02:42:08, kinuko wrote: > > > Drive-by question (probaby to tzik): when are we adding existing batch sync > > > origins (that are not yet migrated in drive_metadata) to > > > pending_batch_sync_origins? (Just want to make sure) > > > > > > It'be nice how batch sync origin changes before and after this patch. > > > > I'm not sure I understand your question. If you're referring to why removing > > this code block doesn't break the current behavior than I believe that I > already > > put the answer in the issue description. > > > > Instead of restoring whole origins back into pending_batch_sync_origins_ the > > files belonging to that origin have their to_be_fetched flag set as true. They > > are then restored in lines 447 - 460. The files have their to_be_fetch bit set > > in DidGetDirectoryContentForBatchSync. > > > > In the case that Chrome crashes before the files for the origin are added as > to > > be fetched, they will only be picked up again when the extension is reloaded > and > > the origin is added as if it was new. > > I think I'm asking about the latter case, and my previous comment (for line > 2194) was about my concern that it doesn't look it's always added back at a > quick glance. > > We may add back the origin at line 753 or at line 2194, but line 752 will return > false if the origin is already in batch-sync-origin list and we only go through > line 2194 if the origin's resource_id has not been fetched yet (but it's usually > not true for already-registered batch sync origins)? I'm happy to be corrected. > > > Does this answer your question? > Right, if a origin is already registered as a BatchSyncOrigin, pending_batch_sync_origin_ will not work. I think we should just drop BatchSyncOrigin from DriveMetadataStore soon after the database is initialized. https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:451: AppendFetchChange(url.origin(), url.path(), resource_id, file_type); Could you add a check to ensure the origin() is incremental sync origin? And if not, drop the change from the database. # This is a fix for the comment around line 836. https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:836: If Chrome crashed around here, the database will turn to wrong state. That is, the database will contain a DriveMetadata without know origin. Putting all these change into single WriteBatch will solve the problem. Could you leave a TODO to do so?
https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (left): https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:445: } On 2013/05/20 04:06:22, kinuko wrote: > On 2013/05/20 03:53:14, calvinlo wrote: > > On 2013/05/20 02:42:08, kinuko wrote: > > > Drive-by question (probaby to tzik): when are we adding existing batch sync > > > origins (that are not yet migrated in drive_metadata) to > > > pending_batch_sync_origins? (Just want to make sure) > > > > > > It'be nice how batch sync origin changes before and after this patch. > > > > I'm not sure I understand your question. If you're referring to why removing > > this code block doesn't break the current behavior than I believe that I > already > > put the answer in the issue description. > > > > Instead of restoring whole origins back into pending_batch_sync_origins_ the > > files belonging to that origin have their to_be_fetched flag set as true. They > > are then restored in lines 447 - 460. The files have their to_be_fetch bit set > > in DidGetDirectoryContentForBatchSync. > > > > In the case that Chrome crashes before the files for the origin are added as > to > > be fetched, they will only be picked up again when the extension is reloaded > and > > the origin is added as if it was new. > > I think I'm asking about the latter case, and my previous comment (for line > 2194) was about my concern that it doesn't look it's always added back at a > quick glance. > > We may add back the origin at line 753 or at line 2194, but line 752 will return > false if the origin is already in batch-sync-origin list and we only go through > line 2194 if the origin's resource_id has not been fetched yet (but it's usually > not true for already-registered batch sync origins)? I'm happy to be corrected. > > > Does this answer your question? > It should get added back at line 753. Line 752 should return false because the origin isn't in DriveMetadataStore.batch_sync_origins_. This is true because the original line 762 was removed. Effectively this CL renders DriveMetadataStore.batch_sync_origins_ unused except on line 852 where it's use temporarily to move the origin directly to incremental status.
https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (left): https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:445: } On 2013/05/20 04:31:54, calvinlo wrote: > On 2013/05/20 04:06:22, kinuko wrote: > > On 2013/05/20 03:53:14, calvinlo wrote: > > > On 2013/05/20 02:42:08, kinuko wrote: > > > > Drive-by question (probaby to tzik): when are we adding existing batch > sync > > > > origins (that are not yet migrated in drive_metadata) to > > > > pending_batch_sync_origins? (Just want to make sure) > > > > > > > > It'be nice how batch sync origin changes before and after this patch. > > > > > > I'm not sure I understand your question. If you're referring to why removing > > > this code block doesn't break the current behavior than I believe that I > > already > > > put the answer in the issue description. > > > > > > Instead of restoring whole origins back into pending_batch_sync_origins_ the > > > files belonging to that origin have their to_be_fetched flag set as true. > They > > > are then restored in lines 447 - 460. The files have their to_be_fetch bit > set > > > in DidGetDirectoryContentForBatchSync. > > > > > > In the case that Chrome crashes before the files for the origin are added as > > to > > > be fetched, they will only be picked up again when the extension is reloaded > > and > > > the origin is added as if it was new. > > > > I think I'm asking about the latter case, and my previous comment (for line > > 2194) was about my concern that it doesn't look it's always added back at a > > quick glance. > > > > We may add back the origin at line 753 or at line 2194, but line 752 will > return > > false if the origin is already in batch-sync-origin list and we only go > through > > line 2194 if the origin's resource_id has not been fetched yet (but it's > usually > > not true for already-registered batch sync origins)? I'm happy to be > corrected. > > > > > Does this answer your question? > > > > It should get added back at line 753. Line 752 should return false because the > origin isn't in DriveMetadataStore.batch_sync_origins_. This is true because the > original line 762 was removed. Effectively this CL renders > DriveMetadataStore.batch_sync_origins_ unused except on line 852 where it's use > temporarily to move the origin directly to incremental status. For a fresh install it should work, but Chrome can get auto-updated while the DB may have batch_sync_origins, can't it?
My comment was written taking into consideration taiju's comment to: "I think we should just drop BatchSyncOrigin from DriveMetadataStore soon after the database is initialized." I will make the changes tzik suggested soon. Thanks. On Mon, May 20, 2013 at 1:41 PM, <kinuko@chromium.org> wrote: > > https://codereview.chromium.**org/15023022/diff/40001/** > chrome/browser/sync_file_**system/drive_file_sync_**service.cc<https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_system/drive_file_sync_service.cc> > File chrome/browser/sync_file_**system/drive_file_sync_**service.cc > (left): > > https://codereview.chromium.**org/15023022/diff/40001/** > chrome/browser/sync_file_**system/drive_file_sync_**service.cc#oldcode445<https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_system/drive_file_sync_service.cc#oldcode445> > chrome/browser/sync_file_**system/drive_file_sync_**service.cc:445: } > On 2013/05/20 04:31:54, calvinlo wrote: > >> On 2013/05/20 04:06:22, kinuko wrote: >> > On 2013/05/20 03:53:14, calvinlo wrote: >> > > On 2013/05/20 02:42:08, kinuko wrote: >> > > > Drive-by question (probaby to tzik): when are we adding existing >> > batch > >> sync >> > > > origins (that are not yet migrated in drive_metadata) to >> > > > pending_batch_sync_origins? (Just want to make sure) >> > > > >> > > > It'be nice how batch sync origin changes before and after this >> > patch. > >> > > >> > > I'm not sure I understand your question. If you're referring to >> > why removing > >> > > this code block doesn't break the current behavior than I believe >> > that I > >> > already >> > > put the answer in the issue description. >> > > >> > > Instead of restoring whole origins back into >> > pending_batch_sync_origins_ the > >> > > files belonging to that origin have their to_be_fetched flag set >> > as true. > >> They >> > > are then restored in lines 447 - 460. The files have their >> > to_be_fetch bit > >> set >> > > in DidGetDirectoryContentForBatch**Sync. >> > > >> > > In the case that Chrome crashes before the files for the origin >> > are added as > >> > to >> > > be fetched, they will only be picked up again when the extension >> > is reloaded > >> > and >> > > the origin is added as if it was new. >> > >> > I think I'm asking about the latter case, and my previous comment >> > (for line > >> > 2194) was about my concern that it doesn't look it's always added >> > back at a > >> > quick glance. >> > >> > We may add back the origin at line 753 or at line 2194, but line 752 >> > will > >> return >> > false if the origin is already in batch-sync-origin list and we only >> > go > >> through >> > line 2194 if the origin's resource_id has not been fetched yet (but >> > it's > >> usually >> > not true for already-registered batch sync origins)? I'm happy to be >> corrected. >> > >> > > Does this answer your question? >> > >> > > It should get added back at line 753. Line 752 should return false >> > because the > >> origin isn't in DriveMetadataStore.batch_sync_**origins_. This is true >> > because the > >> original line 762 was removed. Effectively this CL renders >> DriveMetadataStore.batch_sync_**origins_ unused except on line 852 where >> > it's use > >> temporarily to move the origin directly to incremental status. >> > > For a fresh install it should work, but Chrome can get auto-updated > while the DB may have batch_sync_origins, can't it? > > https://codereview.chromium.**org/15023022/<https://codereview.chromium.org/1... >
Per offline help from Taiju, please note that the review changes while small, forced a lot of other changes so the tests continue to pass and the DB state remains good for previous installs. https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:451: AppendFetchChange(url.origin(), url.path(), resource_id, file_type); On 2013/05/20 04:27:59, tzik wrote: > Could you add a check to ensure the origin() is incremental sync origin? > And if not, drop the change from the database. > # This is a fix for the comment around line 836. Done. https://codereview.chromium.org/15023022/diff/40001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:836: On 2013/05/20 04:27:59, tzik wrote: > If Chrome crashed around here, the database will turn to wrong state. That is, > the database will contain a DriveMetadata without know origin. > > Putting all these change into single WriteBatch will solve the problem. > Could you leave a TODO to do so? Done.
https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:527: if (!metadata_store_->IsBatchSyncOrigin(origin) && This line could be removed too? https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:650: } We seem to be checking pending_batch_sync_origins_ is empty right before we call this method? I guessed this might be added for testing, but if so probably we should change the test code? https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:666: metadata_store_->IsBatchSyncOrigin(origin)) && IsBatchSyncOrigin(origin) is always false, right? https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:735: } nit: no { } for one-line body https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1425: } These lines could be removed too? https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1763: pending_batch_sync_origins_.insert(std::make_pair(origin, resource_id)); These 2 lines are not needed anymore? https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc (right): https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:120: metadata_store()->MoveBatchSyncOriginToIncremental(origin_gurl); It might be less error-prone to just add a new AddIncrementalSyncOrigin() method now (which internally calls these two) and make these two private?
Just FYI, when I started this patch I had talked to Taiju about whether to do the batch sync deprecation in two parts. 1. to render DriveMetadataStore.batch_sync_origins_ unused in favor of DriveFileSyncService.pending_batch_sync_origins. 2. Actually remove DriveMetadataStore.batch_sync_origins_ and associated code. We agreed to do it in 2 parts as to keep the two patches relatively readable. However, because of your drive by review there are now several comments to actually start removing DriveMetadataStore.batch_sync_origins code. I have already done this work in the pending patch (https://codereview.chromium.org/15410005/) which is the follow up to this one. If I continue to make more of Kinuko's suggestions, it might be better to just merge the other pending patch I have into this one. However, it breaks the plan I previously agreed to with Taiju and would make the batch as a whole harder to understand in my opinion. Taiju, can you please confirm what you'd prefer to do i.e. Merge the patches or try to keep them separate still.
I'm not asking for deprecating DriveMetadataStore.batch_sync_origins_ at this stage, just want to make this patch more complete and less error prone on its own. (All I suggest is making consistent changes in DriveFileSyncService while leaving DriveMetadataStore changes in a separate patch) Also if some of my comments don't fit your plan you can just push back each of them with a short comment about the reason. On 2013/05/21 02:09:00, calvinlo wrote: > Just FYI, when I started this patch I had talked to Taiju about whether to do > the batch sync deprecation in two parts. 1. to render > DriveMetadataStore.batch_sync_origins_ unused in favor of > DriveFileSyncService.pending_batch_sync_origins. 2. Actually > remove DriveMetadataStore.batch_sync_origins_ and associated code. > > We agreed to do it in 2 parts as to keep the two patches relatively readable. > However, because of your drive by review there are now several comments to > actually start removing DriveMetadataStore.batch_sync_origins code. I have > already done this work in the pending patch > (https://codereview.chromium.org/15410005/) which is the follow up to this one. > > If I continue to make more of Kinuko's suggestions, it might be better to just > merge the other pending patch I have into this one. However, it breaks the plan > I previously agreed to with Taiju and would make the batch as a whole harder to > understand in my opinion. > > Taiju, can you please confirm what you'd prefer to do i.e. Merge the patches or > try to keep them separate still.
I think it's OK to postpone actual batch sync removal as far as it doesn't break the consistency. Kinuko-san's previous comment pointed out a breakage. For current one, comments on other than UpdateRegisteredOrigin can be put in the next patch. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc (right): https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc:352: DriveMetadataStore::ResourceIdByOrigin::size_type d_size) { these three types can be just size_t. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc:529: base::Passed(&about_resource))); Passed scoped_ptr can be used only once. So, second GetAboutResource seems to cause crash. Can we revert this to WillOnce, if AnyNumber() is not needed?
Sorry for the large number of changes. I've had to change a lot based on review comments, mostly to maintain state consistency. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:527: if (!metadata_store_->IsBatchSyncOrigin(origin) && On 2013/05/20 15:31:50, kinuko wrote: > This line could be removed too? Done. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:650: } On 2013/05/20 15:31:50, kinuko wrote: > We seem to be checking pending_batch_sync_origins_ is empty right before we call > this method? > > I guessed this might be added for testing, but if so probably we should change > the test code? Thanks for catching this. I have removed the errorneous code. I have also adjusted the tests conform to the new behavior. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:666: metadata_store_->IsBatchSyncOrigin(origin)) && On 2013/05/20 15:31:50, kinuko wrote: > IsBatchSyncOrigin(origin) is always false, right? I have added DCHECKS at the beginning to ensure this. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:735: } On 2013/05/20 15:31:50, kinuko wrote: > nit: no { } for one-line body Done. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1425: } On 2013/05/20 15:31:50, kinuko wrote: > These lines could be removed too? Done. I think it's ok to delete these because we move origins directly to incremental sync mode now. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1763: pending_batch_sync_origins_.insert(std::make_pair(origin, resource_id)); On 2013/05/20 15:31:50, kinuko wrote: > These 2 lines are not needed anymore? Done. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc (right): https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc:352: DriveMetadataStore::ResourceIdByOrigin::size_type d_size) { On 2013/05/21 04:05:45, tzik wrote: > these three types can be just size_t. Done. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc:529: base::Passed(&about_resource))); On 2013/05/21 04:05:45, tzik wrote: > Passed scoped_ptr can be used only once. So, second GetAboutResource seems to > cause crash. > Can we revert this to WillOnce, if AnyNumber() is not needed? Done. https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc (right): https://codereview.chromium.org/15023022/diff/70003/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:120: metadata_store()->MoveBatchSyncOriginToIncremental(origin_gurl); On 2013/05/20 15:31:50, kinuko wrote: > It might be less error-prone to just add a new AddIncrementalSyncOrigin() method > now (which internally calls these two) and make these two private? Done. https://codereview.chromium.org/15023022/diff/91009/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/91009/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:394: Do we have a standard way to handle DB migrations? I think this code block might be better if it's wrapped into its own function like DriveMetadataStore.DepricateBatchOrigins() and have it called from DriveMetadataStore.Initialize() So DriveMetadataStore.Initialize() first calls to remove all batch sync origins from the DB before it goes on doing its normal initialization.
lgtm https://codereview.chromium.org/15023022/diff/91009/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/15023022/diff/91009/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:394: On 2013/05/21 07:12:26, calvinlo wrote: > Do we have a standard way to handle DB migrations? I think this code block might > be better if it's wrapped into its own function like > DriveMetadataStore.DepricateBatchOrigins() and have it called from > DriveMetadataStore.Initialize() > > So DriveMetadataStore.Initialize() first calls to remove all batch sync origins > from the DB before it goes on doing its normal initialization. DriveMetadataDB has MigrateDatabaseIfNeeded, and incompatible database change should have migration code around there.
On 2013/05/21 14:08:38, tzik wrote: > lgtm > > https://codereview.chromium.org/15023022/diff/91009/chrome/browser/sync_file_... > File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): > > https://codereview.chromium.org/15023022/diff/91009/chrome/browser/sync_file_... > chrome/browser/sync_file_system/drive_file_sync_service.cc:394: > On 2013/05/21 07:12:26, calvinlo wrote: > > Do we have a standard way to handle DB migrations? I think this code block > might > > be better if it's wrapped into its own function like > > DriveMetadataStore.DepricateBatchOrigins() and have it called from > > DriveMetadataStore.Initialize() > > > > So DriveMetadataStore.Initialize() first calls to remove all batch sync > origins > > from the DB before it goes on doing its normal initialization. > > DriveMetadataDB has MigrateDatabaseIfNeeded, and incompatible database change > should have migration code around there. Ah, the code I have as is will work but I think it's kind of dirty. Should I increment kCurrentDatabaseVersion to 2 and add a MigrateFromVersion1Database() function to just wipe out all the batch_sync_origin entries from the DB? I'm thinking that's probably the best route to do right now.
https://codereview.chromium.org/15023022/diff/121001/chrome/browser/sync_file... File chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc (right): https://codereview.chromium.org/15023022/diff/121001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc:751: metadata_store()->AddIncrementalSyncOrigin(kOrigin2, kDirectoryResourceId2); Could you move this back before SetUpDriveSyncService? Previous ordering emulated the situation that DriveFileSyncService is initialized on existing database. https://codereview.chromium.org/15023022/diff/121001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc:952: EXPECT_EQ(1u, pending_batch_sync_origins()->size()); Can we revert this?
https://codereview.chromium.org/15023022/diff/121001/chrome/browser/sync_file... File chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc (right): https://codereview.chromium.org/15023022/diff/121001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc:751: metadata_store()->AddIncrementalSyncOrigin(kOrigin2, kDirectoryResourceId2); On 2013/05/23 04:03:05, tzik wrote: > Could you move this back before SetUpDriveSyncService? > Previous ordering emulated the situation that DriveFileSyncService is > initialized on existing database. Done. https://codereview.chromium.org/15023022/diff/121001/chrome/browser/sync_file... chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc:952: EXPECT_EQ(1u, pending_batch_sync_origins()->size()); On 2013/05/23 04:03:05, tzik wrote: > Can we revert this? Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calvinlo@chromium.org/15023022/146001
Message was sent while issue was closed.
Change committed as 202025 |
