[Sync] Investigate partial failures and backoff |
|||||
Issue description
Right now USER_EVENTS are receiving partial failures from the server, an example response looks like
{
"client_command": {
"max_commit_batch_size": "90",
"sessions_commit_delay_seconds": "11",
"set_sync_long_poll_interval": "21600",
"set_sync_poll_interval": "14400"
},
"commit": {
"entryresponse": [
{
"error_message": "Backend failure.",
"response_type": "TRANSIENT_ERROR"
}
]
},
"error_code": "SUCCESS",
"store_birthday": "z00000151-c718-6fb0-0000-000055c3f4b3"
}
It seems to me to be throttling all commits, even if we had a commit with some successes and some failures. We are not currently setting ClientToServerResponse::Error, which the current code may expect. It is especially difficult for USER_EVENTS to use the Error message, since they don't have ids.
We need to make sure that:
1. Types with TRANSIENT_ERROR are throttled.
2. Other types are not throttled.
3. Lack of ClientToServerResponse::Error is okay
4. sync-internals communicates the throttling somehow in an easy to read manner.
,
Jul 28 2017
It seems that the client code is expecting "partial failures" to come through ClientToServerResponse::Error, not ClientToServerResponse::CommitResponse::EntryResponse, and is treating an EntryResponse::response_type of TRANSIENT_ERROR as a global backoff. It is unclear what the client behavior should be in the case, we may need to re-think how we want to communicate and coordinate errors between client and server. My initial comment about ClientToServerResponse::Error being difficult to use since USER_EVENTS doesn't send id was incorrect, ClientToServerResponse::Error::error_data_type_ids holds the ids of the data types, not the entries. Lastly, in regards to improving sync-internals, it seems that the client differentiates between "backoff" and "throttled", which is okay, but we're only displaying a boolean for throttled, should be simple enough to add one for backoff as well.
,
Aug 2 2017
,
Aug 7 2017
,
Aug 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a7e43c26d697a994c50c82fa5dfb81c24f44351 commit 9a7e43c26d697a994c50c82fa5dfb81c24f44351 Author: Sky Malice <skym@chromium.org> Date: Thu Aug 10 18:57:58 2017 [Sync] Improve sync-internals throttle reporting. Previously the reporting did not include global backoff and the timing of everything was confusing. Now RestartWaiting() will update both types that are backoff/throttled, as well as the next time we retry. RestartWaiting() is nice because it's always called. In the bug it is stated that we should add backoff/throttled types to sync-internals, but the TypeInfo grid actually already contains a place for this information. Arguably, the Throttled and/or Backoff boolean in the network section is fairly useless when you manually go to sync-internals, but for crash dumps that don't have time for the async type specific information to return it is helpful, so I kept it around. Bug: 750142 Change-Id: I8d5ab4954b829379d7239686ea7c26ef18f770ae Reviewed-on: https://chromium-review.googlesource.com/606558 Reviewed-by: Gang Wu <gangwu@chromium.org> Commit-Queue: Sky Malice <skym@chromium.org> Cr-Commit-Position: refs/heads/master@{#493482} [modify] https://crrev.com/9a7e43c26d697a994c50c82fa5dfb81c24f44351/components/sync/driver/about_sync_util.cc [modify] https://crrev.com/9a7e43c26d697a994c50c82fa5dfb81c24f44351/components/sync/engine_impl/cycle/sync_cycle.cc [modify] https://crrev.com/9a7e43c26d697a994c50c82fa5dfb81c24f44351/components/sync/engine_impl/cycle/sync_cycle.h [modify] https://crrev.com/9a7e43c26d697a994c50c82fa5dfb81c24f44351/components/sync/engine_impl/sync_scheduler_impl.cc [modify] https://crrev.com/9a7e43c26d697a994c50c82fa5dfb81c24f44351/components/sync/engine_impl/sync_scheduler_impl.h [modify] https://crrev.com/9a7e43c26d697a994c50c82fa5dfb81c24f44351/components/sync/engine_impl/sync_scheduler_impl_unittest.cc [modify] https://crrev.com/9a7e43c26d697a994c50c82fa5dfb81c24f44351/components/sync/engine_impl/syncer_unittest.cc [modify] https://crrev.com/9a7e43c26d697a994c50c82fa5dfb81c24f44351/components/sync/test/engine/fake_sync_scheduler.cc [modify] https://crrev.com/9a7e43c26d697a994c50c82fa5dfb81c24f44351/components/sync/test/engine/fake_sync_scheduler.h
,
Aug 10 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by s...@chromium.org
, Jul 28 2017