Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416083002:
[Offline pages] Introduction of MarkAttemptStartedTask with tests (Closed)
|
Created:
4 years, 2 months ago by fgorski Modified:
4 years, 2 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Introduction of MarkAttemptStartedTask with tests
* Creates MarkAttemptStartedTask with tests
* Adds RequestQueue::MarkAttemptStated with tests
* Calls RequestQueue::MarkAttemptStated from RequestCoordinator
BUG=651238
Committed: https://crrev.com/8f70872409d6d8727bdba390c48fbe9c78de3c53
Cr-Commit-Position: refs/heads/master@{#426271}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Addressing code review feedback #Dependent Patchsets: Messages
Total messages: 28 (11 generated)
The CQ bit was checked by fgorski@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...
fgorski@chromium.org changed reviewers: + dougarnett@chromium.org, petewil@chromium.org
Hey, I think I got it to work according to existing behavior. I was wondering what we do if the offliner does not accept the task. Am I missing some code in the baseline? Please take a look and advise. thanks. filip
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:583: // TODO(fgorski): We should probably mark attempt failed here. How is that Unfortunately we do not know if it is a failure or not (PrerenderManager doesn't tell us). Possibly it just was not accepted at this time (previously this was most common for the RECENTLY_VISITED check - but we have resolved that one). Couple other cases we conceivable could hit are TOO_MANY_PROCESSES and RATE_LIMIT_EXCEEDED. So we just count as a start attempt up above for now so that can be retried but not indefinitely.
https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/mark_attempt_started_task.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/mark_attempt_started_task.cc:45: // TODO(petewil): Is there any extra verification we want to do here? I can't think of any verification for starting a task. When we finish a task, it would be good to check that the task actually got started, and DCHECK if it wasn't. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/mark_attempt_started_task.cc:46: read_result->updated_items[0].MarkAttemptStarted(base::Time::Now()); I'm a bit surprised that updated_items isn't const, and we aren't making a copy (I'm fine with it, just a bit surprised). https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/mark_attempt_started_task.h (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/mark_attempt_started_task.h:39: // Store that this task updates. Maybe add "unowned" to the comment. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/mark_attempt_started_task_unittest.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/mark_attempt_started_task_unittest.cc:55: void MarkAttemptStartedTaskTest::AddItemsToStore(RequestQueueStore* store) { Maybe AddItemToStore, since it only adds one item (that surprised me when reading the MarkAttemptStartedWhenItemMissing test). https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:540: // Update the request for this attempt to start it. Comment needs to be updated to say "and then send it to the offliner" https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:552: if (update_result->store_state != StoreState::LOADED || I think our original decision was to keep processing even if the request could not be marked as started. Marking as started serves primarily to prevent crash loops, and this failing should be extremely rare. The primary causes of this failing would be if the request database were unavailable, or the request was not found. However, in either case, we would not have found the request to start processing on it. Maybe logging here is enough? https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:567: // TODO(fgorski): Drop the in-memory caching. I don't understand this comment. Are you suggesting we deprecate the in-memory store? Unit tests still use that, but they could presumably be switched to the SQL store. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:269: void SendRequestToOffliner(const SavePageRequest& request); Should this be another task to both mark started and send to offliner? (Apologies if that is part of the design, and you just haven't coded it yet).
https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:269: void SendRequestToOffliner(const SavePageRequest& request); On 2016/10/14 17:02:35, Pete Williamson wrote: > Should this be another task to both mark started and send to offliner? > (Apologies if that is part of the design, and you just haven't coded it yet). But I don't think this does any other queue/store operations besides MarkStarted so seems good to just have the queue Task cover the db transaction-ish scope but not more.
Responding to a few comments, without making the code changes yet, because I am working on something else. I'll gladly continue the discussion thou. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/mark_attempt_started_task.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/mark_attempt_started_task.cc:45: // TODO(petewil): Is there any extra verification we want to do here? On 2016/10/14 17:02:35, Pete Williamson wrote: > I can't think of any verification for starting a task. When we finish a task, > it would be good to check that the task actually got started, and DCHECK if it > wasn't. This is up to the caller to check/decide/report. We return that information as a valid result. DCHECK would be more appropriate if the function was called with incorrect parameters. In the case stated by you nothing like that happens. Simply some thing failed and informs the caller about that. Removing a TODO https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/mark_attempt_started_task.cc:46: read_result->updated_items[0].MarkAttemptStarted(base::Time::Now()); On 2016/10/14 17:02:35, Pete Williamson wrote: > I'm a bit surprised that updated_items isn't const, and we aren't making a copy > (I'm fine with it, just a bit surprised). We are given our own result, that we are consuming here. There is nothing that needs to be const about it, and nothing else will use it. At the end of this method the result will be deleted. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:567: // TODO(fgorski): Drop the in-memory caching. On 2016/10/14 17:02:35, Pete Williamson wrote: > I don't understand this comment. Are you suggesting we deprecate the in-memory > store? Unit tests still use that, but they could presumably be switched to the > SQL store. this is explicitly about active_request_ which is a single item in memory cache... the in memory store stays based on your comment above. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:583: // TODO(fgorski): We should probably mark attempt failed here. How is that On 2016/10/13 22:54:56, dougarnett wrote: > Unfortunately we do not know if it is a failure or not (PrerenderManager doesn't > tell us). > > Possibly it just was not accepted at this time (previously this was most common > for the RECENTLY_VISITED check - but we have resolved that one). Couple other > cases we conceivable could hit are TOO_MANY_PROCESSES and RATE_LIMIT_EXCEEDED. > So we just count as a start attempt up above for now so that can be retried but > not indefinitely. Yeah, thanks for the explanation. I spoke with Pete after sending a patch and he explained the same thing, which leads me to simply removing the TODO. BTW. I appreciate the fact that this is a solid way to communicate with reviewers ;) https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:269: void SendRequestToOffliner(const SavePageRequest& request); On 2016/10/14 20:12:31, dougarnett wrote: > On 2016/10/14 17:02:35, Pete Williamson wrote: > > Should this be another task to both mark started and send to offliner? > > (Apologies if that is part of the design, and you just haven't coded it yet). > No, because right now we are not planning to serialize everything the request coordinator is doing, only the database access (through request queue). If as a result of offlining we are getting into situation where something needs to be written, will will call request queue from there to schedule a relevant task. > But I don't think this does any other queue/store operations besides MarkStarted > so seems good to just have the queue Task cover the db transaction-ish scope but > not more. And I think Doug provides similar explanation here.
https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:567: // TODO(fgorski): Drop the in-memory caching. On 2016/10/14 20:37:19, fgorski wrote: > On 2016/10/14 17:02:35, Pete Williamson wrote: > > I don't understand this comment. Are you suggesting we deprecate the > in-memory > > store? Unit tests still use that, but they could presumably be switched to > the > > SQL store. > > this is explicitly about active_request_ which is a single item in memory > cache... the in memory store stays based on your comment above. Yes, we want to set active_request_ if we call LoadAndSave (or especially if it returns true). I am confused at the moment though why we might want it set if LoadAndSave returns false. Pete, can you think if this is intentional?
https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:567: // TODO(fgorski): Drop the in-memory caching. On 2016/10/14 20:59:04, dougarnett wrote: > On 2016/10/14 20:37:19, fgorski wrote: > > On 2016/10/14 17:02:35, Pete Williamson wrote: > > > I don't understand this comment. Are you suggesting we deprecate the > > in-memory > > > store? Unit tests still use that, but they could presumably be switched to > > the > > > SQL store. > > > > this is explicitly about active_request_ which is a single item in memory > > cache... the in memory store stays based on your comment above. > > Yes, we want to set active_request_ if we call LoadAndSave (or especially if it > returns true). I am confused at the moment though why we might want it set if > LoadAndSave returns false. Pete, can you think if this is intentional? Doug, you confirmed what the code does right now, but you are not explaining why we need that and it's really the most important part. Please, explain.
https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:567: // TODO(fgorski): Drop the in-memory caching. On 2016/10/17 14:22:13, fgorski wrote: > On 2016/10/14 20:59:04, dougarnett wrote: > > On 2016/10/14 20:37:19, fgorski wrote: > > > On 2016/10/14 17:02:35, Pete Williamson wrote: > > > > I don't understand this comment. Are you suggesting we deprecate the > > > in-memory > > > > store? Unit tests still use that, but they could presumably be switched > to > > > the > > > > SQL store. > > > > > > this is explicitly about active_request_ which is a single item in memory > > > cache... the in memory store stays based on your comment above. > > > > Yes, we want to set active_request_ if we call LoadAndSave (or especially if > it > > returns true). I am confused at the moment though why we might want it set if > > LoadAndSave returns false. Pete, can you think if this is intentional? > > Doug, you confirmed what the code does right now, but you are not explaining why > we need that and it's really the most important part. Please, explain. It tracks state of pending operation and which request it is for potential canceling or aborting and UMA. The request_id is used to decide whether a Paused or Removed request should cause Offliner to be canceled. The namespace, client_id, and creation_time are use for UMA. The request itself is used for MarkAttemptAborted. So using static information plus handle to make that MarkAttemptAborted call. If we can put the mark aborted and uma within a Task, them probably could just cache request_id.
On 2016/10/17 16:36:58, dougarnett wrote: > https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... > File components/offline_pages/background/request_coordinator.cc (right): > > https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... > components/offline_pages/background/request_coordinator.cc:567: // > TODO(fgorski): Drop the in-memory caching. > On 2016/10/17 14:22:13, fgorski wrote: > > On 2016/10/14 20:59:04, dougarnett wrote: > > > On 2016/10/14 20:37:19, fgorski wrote: > > > > On 2016/10/14 17:02:35, Pete Williamson wrote: > > > > > I don't understand this comment. Are you suggesting we deprecate the > > > > in-memory > > > > > store? Unit tests still use that, but they could presumably be switched > > to > > > > the > > > > > SQL store. > > > > > > > > this is explicitly about active_request_ which is a single item in memory > > > > cache... the in memory store stays based on your comment above. > > > > > > Yes, we want to set active_request_ if we call LoadAndSave (or especially if > > it > > > returns true). I am confused at the moment though why we might want it set > if > > > LoadAndSave returns false. Pete, can you think if this is intentional? > > > > Doug, you confirmed what the code does right now, but you are not explaining > why > > we need that and it's really the most important part. Please, explain. > > It tracks state of pending operation and which request it is for potential > canceling or aborting and UMA. The request_id is used to decide whether a > Paused or Removed request should cause Offliner to be canceled. The namespace, > client_id, and creation_time are use for UMA. The request itself is used for > MarkAttemptAborted. So using static information plus handle to make that > MarkAttemptAborted call. If we can put the mark aborted and uma within a Task, > them probably could just cache request_id. MarkAttemptAborted only requires request_id, once it uses a task, so we are good there. I was originally thinking of keeping only request_id, but you make a very good point about UMA. I wonder if we could limit the information we keep, by currying or auditing ourselves and never writing the information back. (As that is how we run into problems.)
https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:567: // TODO(fgorski): Drop the in-memory caching. On 2016/10/17 16:36:58, dougarnett wrote: > On 2016/10/17 14:22:13, fgorski wrote: > > On 2016/10/14 20:59:04, dougarnett wrote: > > > On 2016/10/14 20:37:19, fgorski wrote: > > > > On 2016/10/14 17:02:35, Pete Williamson wrote: > > > > > I don't understand this comment. Are you suggesting we deprecate the > > > > in-memory > > > > > store? Unit tests still use that, but they could presumably be switched > > to > > > > the > > > > > SQL store. > > > > > > > > this is explicitly about active_request_ which is a single item in memory > > > > cache... the in memory store stays based on your comment above. > > > > > > Yes, we want to set active_request_ if we call LoadAndSave (or especially if > > it > > > returns true). I am confused at the moment though why we might want it set > if > > > LoadAndSave returns false. Pete, can you think if this is intentional? > > > > Doug, you confirmed what the code does right now, but you are not explaining > why > > we need that and it's really the most important part. Please, explain. > > It tracks state of pending operation and which request it is for potential > canceling or aborting and UMA. The request_id is used to decide whether a > Paused or Removed request should cause Offliner to be canceled. The namespace, > client_id, and creation_time are use for UMA. The request itself is used for > MarkAttemptAborted. So using static information plus handle to make that > MarkAttemptAborted call. If we can put the mark aborted and uma within a Task, > them probably could just cache request_id. Please clarify the TODO wrt your idea of only caching the request_id instead of the full request so we are more clear about the idea. We can later figure out how to deal with caching or querying the details needed for UMA when we look to DO the TODO.
The CQ bit was checked by fgorski@chromium.org to run a CQ dry run
Comments addressed. PTAL https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/mark_attempt_started_task.h (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/mark_attempt_started_task.h:39: // Store that this task updates. On 2016/10/14 17:02:35, Pete Williamson wrote: > Maybe add "unowned" to the comment. Done. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/mark_attempt_started_task_unittest.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/mark_attempt_started_task_unittest.cc:55: void MarkAttemptStartedTaskTest::AddItemsToStore(RequestQueueStore* store) { On 2016/10/14 17:02:35, Pete Williamson wrote: > Maybe AddItemToStore, since it only adds one item (that surprised me when > reading the MarkAttemptStartedWhenItemMissing test). Done. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:540: // Update the request for this attempt to start it. On 2016/10/14 17:02:35, Pete Williamson wrote: > Comment needs to be updated to say "and then send it to the offliner" Done. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:552: if (update_result->store_state != StoreState::LOADED || On 2016/10/14 17:02:35, Pete Williamson wrote: > I think our original decision was to keep processing even if the request could > not be marked as started. Marking as started serves primarily to prevent crash > loops, and this failing should be extremely rare. > > The primary causes of this failing would be if the request database were > unavailable, or the request was not found. However, in either case, we would > not have found the request to start processing on it. Maybe logging here is > enough? What if request is not in database? What if the store is corrupted? This catches such cases. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:567: // TODO(fgorski): Drop the in-memory caching. On 2016/10/17 18:59:44, dougarnett wrote: > On 2016/10/17 16:36:58, dougarnett wrote: > > On 2016/10/17 14:22:13, fgorski wrote: > > > On 2016/10/14 20:59:04, dougarnett wrote: > > > > On 2016/10/14 20:37:19, fgorski wrote: > > > > > On 2016/10/14 17:02:35, Pete Williamson wrote: > > > > > > I don't understand this comment. Are you suggesting we deprecate the > > > > > in-memory > > > > > > store? Unit tests still use that, but they could presumably be > switched > > > to > > > > > the > > > > > > SQL store. > > > > > > > > > > this is explicitly about active_request_ which is a single item in > memory > > > > > cache... the in memory store stays based on your comment above. > > > > > > > > Yes, we want to set active_request_ if we call LoadAndSave (or especially > if > > > it > > > > returns true). I am confused at the moment though why we might want it set > > if > > > > LoadAndSave returns false. Pete, can you think if this is intentional? > > > > > > Doug, you confirmed what the code does right now, but you are not explaining > > why > > > we need that and it's really the most important part. Please, explain. > > > > It tracks state of pending operation and which request it is for potential > > canceling or aborting and UMA. The request_id is used to decide whether a > > Paused or Removed request should cause Offliner to be canceled. The namespace, > > client_id, and creation_time are use for UMA. The request itself is used for > > MarkAttemptAborted. So using static information plus handle to make that > > MarkAttemptAborted call. If we can put the mark aborted and uma within a Task, > > them probably could just cache request_id. > > Please clarify the TODO wrt your idea of only caching the request_id instead > of the full request so we are more clear about the idea. We can later figure > out how to deal with caching or querying the details needed for UMA when we > look to DO the TODO. Done. https://codereview.chromium.org/2416083002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:583: // TODO(fgorski): We should probably mark attempt failed here. How is that On 2016/10/14 20:37:19, fgorski wrote: > On 2016/10/13 22:54:56, dougarnett wrote: > > Unfortunately we do not know if it is a failure or not (PrerenderManager > doesn't > > tell us). > > > > Possibly it just was not accepted at this time (previously this was most > common > > for the RECENTLY_VISITED check - but we have resolved that one). Couple other > > cases we conceivable could hit are TOO_MANY_PROCESSES and RATE_LIMIT_EXCEEDED. > > So we just count as a start attempt up above for now so that can be retried > but > > not indefinitely. > > Yeah, thanks for the explanation. I spoke with Pete after sending a patch and he > explained the same thing, which leads me to simply removing the TODO. BTW. I > appreciate the fact that this is a solid way to communicate with reviewers ;) Done.
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: Exceeded global retry quota
lgtm
lgtm
lgtm
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Introduction of MarkAttemptStartedTask with tests * Creates MarkAttemptStartedTask with tests * Adds RequestQueue::MarkAttemptStated with tests * Calls RequestQueue::MarkAttemptStated from RequestCoordinator BUG=651238 ========== to ========== [Offline pages] Introduction of MarkAttemptStartedTask with tests * Creates MarkAttemptStartedTask with tests * Adds RequestQueue::MarkAttemptStated with tests * Calls RequestQueue::MarkAttemptStated from RequestCoordinator BUG=651238 Committed: https://crrev.com/8f70872409d6d8727bdba390c48fbe9c78de3c53 Cr-Commit-Position: refs/heads/master@{#426271} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8f70872409d6d8727bdba390c48fbe9c78de3c53 Cr-Commit-Position: refs/heads/master@{#426271} |
