Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2607443002:
Count and report number of duplicated files found during import. (Closed)
|
Created:
3 years, 12 months ago by yamaguchi Modified:
3 years, 11 months ago CC:
chromium-reviews, tfarina, yamaguchi+watch_chromium.org, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCount and report number of duplicated files found during import.
Issue 2594523002 has changed the scan process for Cloud Backup.
The directory scan and selection scan no longer reports before import
content_duplicate files. This patch instead counts the number of such
duplicated files found during the import task and adds it as
ImprotEvents.CONTENT_DUPLICATE metric.
BUG=668574
TEST=browser_test FileManagerJsTest.MediaImportHandlerTest
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2607443002
Cr-Commit-Position: refs/heads/master@{#444618}
Committed: https://chromium.googlesource.com/chromium/src/+/d31e43ce6af3cea0654218a0d4db7188addb71d3
Patch Set 1 #
Total comments: 5
Patch Set 2 : Replace stats for CONTENT_DUPLICATE outside the loop after making sure it exists and equals to zero. #Patch Set 3 : Rebase to head #Patch Set 4 : Fix indent. #Messages
Total messages: 31 (21 generated)
Description was changed from ========== Count and report number of duplicated files found during import. BUG=668574 ========== to ========== Count and report number of duplicated files found during import. BUG=668574 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Count and report number of duplicated files found during import. BUG=668574 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Count and report number of duplicated files found during import. BUG=668574 TEST=browser_test FileManagerJsTest.MediaImportHandlerTest CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Count and report number of duplicated files found during import. BUG=668574 TEST=browser_test FileManagerJsTest.MediaImportHandlerTest CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Count and report number of duplicated files found during import. Issue 2594523002 has changed the scan process for Cloud Backup. The directory scan and selection scan no longer reports before import content_duplicate files. This patch instead counts the number of such duplicated files found during the import task and adds it as ImprotEvents.CONTENT_DUPLICATE metric. BUG=668574 TEST=browser_test FileManagerJsTest.MediaImportHandlerTest CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
yamaguchi@chromium.org changed reviewers: + fukino@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2607443002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/background/js/media_import_handler.js (right): https://codereview.chromium.org/2607443002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/background/js/media_import_handler.js:600: count += this.duplicateFilesCount_; Shouldn't we reset the duplicateFilesCount_ here?
https://codereview.chromium.org/2607443002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/background/js/media_import_handler.js (right): https://codereview.chromium.org/2607443002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/background/js/media_import_handler.js:600: count += this.duplicateFilesCount_; On 2016/12/26 10:34:46, fukino wrote: > Shouldn't we reset the duplicateFilesCount_ here? I don't think we need to reset the counter because this function should be run only once after the import task is finished. errorCount_ and scanResult_ are handled in the same way.
lgtm https://codereview.chromium.org/2607443002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/background/js/media_import_handler.js (right): https://codereview.chromium.org/2607443002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/background/js/media_import_handler.js:600: count += this.duplicateFilesCount_; On 2016/12/26 11:35:03, yamaguchi wrote: > On 2016/12/26 10:34:46, fukino wrote: > > Shouldn't we reset the duplicateFilesCount_ here? > > I don't think we need to reset the counter because this function should be run > only once after the import task is finished. errorCount_ and scanResult_ are > handled in the same way. sgtm. Thank you for the explanation.
yamaguchi@chromium.org changed reviewers: + smckay@chromium.org
+smckay Will you review the change for the analytics?
Added some comments, but please feel free to submit w/ Fukino-san's LGTM. https://codereview.chromium.org/2607443002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/background/js/media_import_handler.js (right): https://codereview.chromium.org/2607443002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/background/js/media_import_handler.js:597: if (disposition == importer.Disposition.CONTENT_DUPLICATE) { If we're not doing content dedupe in the scan phase, it's weird that we're relying on an entry for that disposition in the scan stats object. I can imagine how this could easily regress by that disposition not being present at all in scan stats. So maybe we should be more explicit about how we handle mixing-in content dupes stats. E.g. 1) assert it is present 2) assert it is zero Another question....were we only ever sending scan stats when a user performed an import?
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2607443002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/background/js/media_import_handler.js (right): https://codereview.chromium.org/2607443002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/background/js/media_import_handler.js:597: if (disposition == importer.Disposition.CONTENT_DUPLICATE) { On 2016/12/27 16:59:43, Steve McKay wrote: > If we're not doing content dedupe in the scan phase, it's weird that we're > relying on an entry for that disposition in the scan stats object. I can imagine > how this could easily regress by that disposition not being present at all in > scan stats. > > So maybe we should be more explicit about how we handle mixing-in content dupes > stats. E.g. > > 1) assert it is present > 2) assert it is zero I agree. Changed to check the existence of the field and the value. > > Another question....were we only ever sending scan stats when a user performed > an import? Yes. ImportTask has been sending the stats, thus we won't send a scan stats unless it goes on to an import phase. The only exception I know is the code path to do import task without user operation, when a scan found content-duplicate files but no new files. It happened between the changes https://codereview.chromium.org/2564343002/ and https://codereview.chromium.org/2594523002/
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by yamaguchi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from smckay@chromium.org Link to the patchset: https://codereview.chromium.org/2607443002/#ps60001 (title: "Fix indent.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484793683753890,
"parent_rev": "ec812adbe02d56c9efafcaa77c7dfd9121510959", "commit_rev":
"d31e43ce6af3cea0654218a0d4db7188addb71d3"}
Message was sent while issue was closed.
Description was changed from ========== Count and report number of duplicated files found during import. Issue 2594523002 has changed the scan process for Cloud Backup. The directory scan and selection scan no longer reports before import content_duplicate files. This patch instead counts the number of such duplicated files found during the import task and adds it as ImprotEvents.CONTENT_DUPLICATE metric. BUG=668574 TEST=browser_test FileManagerJsTest.MediaImportHandlerTest CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Count and report number of duplicated files found during import. Issue 2594523002 has changed the scan process for Cloud Backup. The directory scan and selection scan no longer reports before import content_duplicate files. This patch instead counts the number of such duplicated files found during the import task and adds it as ImprotEvents.CONTENT_DUPLICATE metric. BUG=668574 TEST=browser_test FileManagerJsTest.MediaImportHandlerTest CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2607443002 Cr-Commit-Position: refs/heads/master@{#444618} Committed: https://chromium.googlesource.com/chromium/src/+/d31e43ce6af3cea0654218a0d4db... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d31e43ce6af3cea0654218a0d4db... |
