Sync USS migrator drops progress marker if zero entities to migrate |
|||||||||
Issue descriptionThe implementation in uss_migrator.cc, which is exercised for all USS rollouts (when a user transitions from directory to USS for a datatype) has a bug that causes the progress marker to be dropped if there are no entities to migrate (or they are all tombstones). This can be easily reproduced for HISTORY_DELETE_DIRECTIVES, because the SyncableService always deletes entries from the directory after processing them, by toggling flag --enable-sync-pseudo-uss-history-delete-directives. In addition, because of implementation details, the worker and processor end up believing that initial sync is already completed (although there is no progress marker), which for example causes the next updates from the server to directly contribute to bucket REMOTE_INCREMENTAL_UPDATES Sync.ModelTypeEntityChange3.<ModelType>.
,
Jan 15
Requesting a merge to M72 beta. The fix has been tested on canary and looks good.
,
Jan 15
This bug requires manual review: Less than 10 days to go before AppStore submit on M72 Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 15
Before we approve merge to M72, please answer followings: * Is this M72 regression? Is it critical? * Is the change having enough automation tests coverage and safe to merge to M72? * Any other important details to justify the merge. Please note M72 is going to stable soon, so merge bar is *very* high. Thank you.
,
Jan 15
> Is this M72 regression? Is it critical? It's not a regression because the feature toggle is not enabled prior to M72. It would however block the launch. > Is the change having enough automation tests coverage and safe to merge to M72? Yes, it has proper test coverage and is safe to merge to M72 because it's behind feature toggle. > Any other important details to justify the merge. The patch is straightforward, behind a feature toggle, and will avoid slipping one milestone.
,
Jan 15
Thank you y mastiz@. Is there a launch bug for this feature and do you have approval for launch?
,
Jan 15
Sorry didn't mean to approve the merge, waiting on reply to #6.
,
Jan 15
Launchbug is https://bugs.chromium.org/p/chromium/issues/approval?id=902704 We have all approvals except leadership approval which we haven't yet requested.
,
Jan 15
Thank you mastiz@. Approving merge to M72 branch 3626 based on comment #2, #5 & #8. Please merge ASAP. Thank you.
,
Jan 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/99ddc3b15ebdd270f7d4f68192d1680f52dc1c29 commit 99ddc3b15ebdd270f7d4f68192d1680f52dc1c29 Author: Mikel Astiz <mastiz@chromium.org> Date: Tue Jan 15 20:03:22 2019 Fix USS migrator potentially dropping progress marker ProcessGetUpdatesResponse() needs to be called at least one in order to update the progress marker as known by the worker (which also gets propagated to the processor) and avoid unnecessary downloads from the sync server. This also avoids the inconsistency of not having a progress marker but still considering initial sync done (which is marked in PassiveApplyUpdates()). The fix is restricted to HISTORY_DELETE_DIRECTIVES for now as a precaution, where the issue is known to be quite relevant (because of how the SyncableService deletes entries after processing them). Follow-up patches will extend this to all datatypes. Bug: 921495 Change-Id: I93bf754bb2b57eb1e462f489a91273c1292f1039 Reviewed-on: https://chromium-review.googlesource.com/c/1408911 Reviewed-by: Jan Krcal <jkrcal@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#622452}(cherry picked from commit b9544bb61fc022eeff8d259231b64900a2398c33) Reviewed-on: https://chromium-review.googlesource.com/c/1412821 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#694} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/99ddc3b15ebdd270f7d4f68192d1680f52dc1c29/components/sync/engine_impl/uss_migrator.cc [modify] https://crrev.com/99ddc3b15ebdd270f7d4f68192d1680f52dc1c29/components/sync/engine_impl/uss_migrator_unittest.cc
,
Jan 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/99ddc3b15ebdd270f7d4f68192d1680f52dc1c29 Commit: 99ddc3b15ebdd270f7d4f68192d1680f52dc1c29 Author: mastiz@chromium.org Commiter: mastiz@chromium.org Date: 2019-01-15 20:03:22 +0000 UTC Fix USS migrator potentially dropping progress marker ProcessGetUpdatesResponse() needs to be called at least one in order to update the progress marker as known by the worker (which also gets propagated to the processor) and avoid unnecessary downloads from the sync server. This also avoids the inconsistency of not having a progress marker but still considering initial sync done (which is marked in PassiveApplyUpdates()). The fix is restricted to HISTORY_DELETE_DIRECTIVES for now as a precaution, where the issue is known to be quite relevant (because of how the SyncableService deletes entries after processing them). Follow-up patches will extend this to all datatypes. Bug: 921495 Change-Id: I93bf754bb2b57eb1e462f489a91273c1292f1039 Reviewed-on: https://chromium-review.googlesource.com/c/1408911 Reviewed-by: Jan Krcal <jkrcal@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#622452}(cherry picked from commit b9544bb61fc022eeff8d259231b64900a2398c33) Reviewed-on: https://chromium-review.googlesource.com/c/1412821 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#694} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 17
(5 days ago)
,
Jan 17
(5 days ago)
Mikel, do you have any bug open for the follow-up fix later (extending the fix to all datatypes)?
,
Jan 17
(5 days ago)
I'll link it to this bug despite it being closed. Let me write that now.
,
Jan 17
(5 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c808ad1786d900a1ee2f8d6fb4f86b94fcfd809e commit c808ad1786d900a1ee2f8d6fb4f86b94fcfd809e Author: Mikel Astiz <mastiz@chromium.org> Date: Thu Jan 17 13:07:13 2019 Fix USS migrator dropping progress marker for all datatypes A previous patch (https://chromium-review.googlesource.com/c/1408911) fixed the issue specifically for datatype HISTORY_DELETE_DIRECTIVES and we now generalize the fix for all. This fix should avoid redundant traffic to sync servers for users transitioning from directory to USS, with the corresponding impact on UMA metrics that we often use for evaluating launches (in particular metric Sync.ModelTypeEntityChange3.<ModelType>. Bug: 921495 Change-Id: Ib3c6d1cae93dd45b64dea62cde82171a37c99558 Reviewed-on: https://chromium-review.googlesource.com/c/1416140 Commit-Queue: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org> Cr-Commit-Position: refs/heads/master@{#623659} [modify] https://crrev.com/c808ad1786d900a1ee2f8d6fb4f86b94fcfd809e/components/sync/engine_impl/uss_migrator.cc [modify] https://crrev.com/c808ad1786d900a1ee2f8d6fb4f86b94fcfd809e/components/sync/engine_impl/uss_migrator_unittest.cc |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bugdroid1@chromium.org
, Jan 14