Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2369063003:
[Sync] Add an error integration test for USS. (Closed)
Description[Sync] Add an error integration test for USS.
Caught two bugs:
- The NonBlockingDTC needs to report errors even if the type is running.
- The DataTypeManagerImpl must do ProcessReconfigure asynchronously
to give the ModelAssociationManager time to finish stopping the type.
BUG=643269
Committed: https://crrev.com/dff5a0d6ec28d34867aa238e0b39aa0386004222
Cr-Commit-Position: refs/heads/master@{#421676}
Patch Set 1 #Patch Set 2 : Rebase + fix DTMI. #Patch Set 3 : Fix a DCHECK in DTMI. #
Total comments: 12
Patch Set 4 : Remove empty destructors. #
Total comments: 2
Patch Set 5 : Improve comments. #
Messages
Total messages: 26 (18 generated)
The CQ bit was checked by maxbogue@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 ========== [Sync] Add an error integration test for USS. Caught a little bug in the NonBlockingDTC; we need to report errors even if the type is running. BUG=643269 ========== to ========== [Sync] Add an error integration test for USS. Caught two bugs: - The NonBlockingDTC needs to report errors even if the type is running. - The DataTypeManagerImpl must do ProcessReconfigure asynchronously to give the ModelAssociationManager time to finish stopping the type. BUG=643269 ==========
maxbogue@chromium.org changed reviewers: + pavely@chromium.org, skym@chromium.org
PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by maxbogue@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.
I'm worried about adding PostTasks to make things work. I'd love an alternative solution, perhaps we can brainstorm tomorrow during our USS meeting. Maybe this is the best approach. https://codereview.chromium.org/2369063003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right): https://codereview.chromium.org/2369063003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:191: ~PrefsNotRunningChecker() override {} I don't think you need this. I'm going through the other checkers are removing these. StatusChangeChecker's dtor is not pure. https://codereview.chromium.org/2369063003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:194: SingleClientStatusChangeChecker::Wait(); This awkwardness should be fixed by my CL. https://codereview.chromium.org/2369063003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:339: model1->WriteItem(kKey1, kValue2); What's the point of this? You don't verify that model2 doesn't have key1->value2. https://codereview.chromium.org/2369063003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:342: ASSERT_TRUE(PrefsNotRunningChecker(GetSyncService(1)).Wait()); Seems odd that you error(2) -> write(1) -> block for shutdown(2). Shouldn't you error(2) -> block for shutdown(2) -> write(1)? https://codereview.chromium.org/2369063003/diff/40001/components/sync/driver/... File components/sync/driver/data_type_manager_impl.cc (right): https://codereview.chromium.org/2369063003/diff/40001/components/sync/driver/... components/sync/driver/data_type_manager_impl.cc:509: base::ThreadTaskRunnerHandle::Get()->PostTask( Is this really the best way to fix this? I worry that sprinkling PostTasks all over the code base is going to make things messy long term. ProcessReconfigure() already has an abort check if model_association_manager_.state() == ModelAssociationManager::ASSOCIATING Maybe we could continue to do something like this. I'm not sure. https://codereview.chromium.org/2369063003/diff/40001/components/sync/driver/... File components/sync/driver/non_blocking_data_type_controller.cc (left): https://codereview.chromium.org/2369063003/diff/40001/components/sync/driver/... components/sync/driver/non_blocking_data_type_controller.cc:221: if (state_ == MODEL_STARTING) { Hmm, I have no idea why this if check was initially written.
The CQ bit was checked by maxbogue@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...
https://codereview.chromium.org/2369063003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right): https://codereview.chromium.org/2369063003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:191: ~PrefsNotRunningChecker() override {} On 2016/09/27 20:24:45, skym wrote: > I don't think you need this. I'm going through the other checkers are removing > these. StatusChangeChecker's dtor is not pure. Removed them all. https://codereview.chromium.org/2369063003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:194: SingleClientStatusChangeChecker::Wait(); On 2016/09/27 20:24:44, skym wrote: > This awkwardness should be fixed by my CL. Acknowledged. https://codereview.chromium.org/2369063003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:339: model1->WriteItem(kKey1, kValue2); On 2016/09/27 20:24:45, skym wrote: > What's the point of this? You don't verify that model2 doesn't have > key1->value2. To trigger an ApplySyncChanges (GU) in model2. https://codereview.chromium.org/2369063003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:342: ASSERT_TRUE(PrefsNotRunningChecker(GetSyncService(1)).Wait()); On 2016/09/27 20:24:44, skym wrote: > Seems odd that you error(2) -> write(1) -> block for shutdown(2). Shouldn't you > error(2) -> block for shutdown(2) -> write(1)? SetServiceError stores the error that is returned on the next fallible service operation. The write is to make sure a service operation happens. https://codereview.chromium.org/2369063003/diff/40001/components/sync/driver/... File components/sync/driver/data_type_manager_impl.cc (right): https://codereview.chromium.org/2369063003/diff/40001/components/sync/driver/... components/sync/driver/data_type_manager_impl.cc:509: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/09/27 20:24:45, skym wrote: > Is this really the best way to fix this? I worry that sprinkling PostTasks all > over the code base is going to make things messy long term. > > ProcessReconfigure() already has an abort check if > > model_association_manager_.state() == > ModelAssociationManager::ASSOCIATING > > Maybe we could continue to do something like this. I'm not sure. Moving forward with this as discussed offline; Pavel will be refactoring this code in the near future. https://codereview.chromium.org/2369063003/diff/40001/components/sync/driver/... File components/sync/driver/non_blocking_data_type_controller.cc (left): https://codereview.chromium.org/2369063003/diff/40001/components/sync/driver/... components/sync/driver/non_blocking_data_type_controller.cc:221: if (state_ == MODEL_STARTING) { On 2016/09/27 20:24:45, skym wrote: > Hmm, I have no idea why this if check was initially written. Who knowssss!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2369063003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right): https://codereview.chromium.org/2369063003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:331: // Queue an error in model 2 and trigger a GetUpdates. As we talked about offline, expand this comment to include all the middle steps and causes.
The CQ bit was checked by maxbogue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/2369063003/#ps80001 (title: "Improve comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2369063003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right): https://codereview.chromium.org/2369063003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:331: // Queue an error in model 2 and trigger a GetUpdates. On 2016/09/28 21:53:34, skym wrote: > As we talked about offline, expand this comment to include all the middle steps > and causes. Done.
Message was sent while issue was closed.
Description was changed from ========== [Sync] Add an error integration test for USS. Caught two bugs: - The NonBlockingDTC needs to report errors even if the type is running. - The DataTypeManagerImpl must do ProcessReconfigure asynchronously to give the ModelAssociationManager time to finish stopping the type. BUG=643269 ========== to ========== [Sync] Add an error integration test for USS. Caught two bugs: - The NonBlockingDTC needs to report errors even if the type is running. - The DataTypeManagerImpl must do ProcessReconfigure asynchronously to give the ModelAssociationManager time to finish stopping the type. BUG=643269 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Add an error integration test for USS. Caught two bugs: - The NonBlockingDTC needs to report errors even if the type is running. - The DataTypeManagerImpl must do ProcessReconfigure asynchronously to give the ModelAssociationManager time to finish stopping the type. BUG=643269 ========== to ========== [Sync] Add an error integration test for USS. Caught two bugs: - The NonBlockingDTC needs to report errors even if the type is running. - The DataTypeManagerImpl must do ProcessReconfigure asynchronously to give the ModelAssociationManager time to finish stopping the type. BUG=643269 Committed: https://crrev.com/dff5a0d6ec28d34867aa238e0b39aa0386004222 Cr-Commit-Position: refs/heads/master@{#421676} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/dff5a0d6ec28d34867aa238e0b39aa0386004222 Cr-Commit-Position: refs/heads/master@{#421676} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
