Add an API to Pause and Resume background offlining requests.

Add an API to Pause and Resume background offlining requests.

The new API, PauseRequestsbyClientId and ResumeRequestsByClientId
is defined and implemented.  Tests are included.

It is basically parallel to the existing RemoveRequestByClientId,
and you can compare them when reviewing to speed up the review
process.

It is implemented all the way down into the request queue,
including both the in-memory queue and the SQL queue.

BUG=610521
Committed: https://crrev.com/48d69bf7b3937d03580c90e06767e0f909dab6be
Cr-Commit-Position: refs/heads/master@{#411726}

[Pete Williamson, 2016-08-09 16:37:31]

petewil@chromium.org changed reviewers:
+ dimich@chromium.org, fgorski@chromium.org

[Pete Williamson, 2016-08-09 16:37:32]

I'm working on the change from ClientId to RequestId in another patch, which
will also update these methods.

[Pete Williamson, 2016-08-09 16:37:39]

The CQ bit was checked by petewil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 16:38:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Pete Williamson, 2016-08-09 17:50:59]

Hold off a bit if you haven't started reviewing this yet, I'm putting back all
the RemoveRequest(byRequestID) methods, that will make the review easier, and
the subsequent merge easier.

[Pete Williamson, 2016-08-09 17:58:11]

OK to review now.

[Pete Williamson, 2016-08-09 18:17:43]

Hold off a bit longer, I'll update functions to use request_ids instead of
client_ids since it appears that the review hasn't started yet.

[Pete Williamson, 2016-08-09 19:54:28]

OK to review now, changed to use RequestId as key instead of ClientId.

[Pete Williamson, 2016-08-09 19:54:35]

The CQ bit was checked by petewil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 19:55:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dmitry Titov, 2016-08-09 21:01:43]

https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
File components/offline_pages/background/request_queue_in_memory_store.cc
(right):

https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:104: for
(auto iter = requests_.begin(); iter != requests_.end(); ++iter) {
I think requests_ is already a map from request_id, so it is enough to just walk
the request_ids and for each id find the request in the requests_...

https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:462: if
(!db_.get()) {
Could you pull all these identical blocks into something like 

if (!checkDb(callback))
  return;

with checkDb having the code to post task etc? and change to it in whole file.

https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:470:
background_task_runner_->PostTask(
Is there a reason we don't use PostTaskAndReply in all these methods?

[commit-bot: I haz the power, 2016-08-09 22:36:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 22:36:40]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-08-09 22:53:24]

https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:462: if
(!db_.get()) {
On 2016/08/09 21:01:43, Dmitry Titov wrote:
> Could you pull all these identical blocks into something like 
> 
> if (!checkDb(callback))
>   return;
> 
> with checkDb having the code to post task etc? and change to it in whole file.

Done.  Unfortunately, the callbacks have different types and take different
parameter sets, so we ended up with 4 CheckDb functions instead of one.

I skipped RemoveRequestByClientId since it is being deleted in a different
changelist (to reduce merge problems later).

https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:470:
background_task_runner_->PostTask(
On 2016/08/09 21:01:43, Dmitry Titov wrote:
> Is there a reason we don't use PostTaskAndReply in all these methods?

I'm following the model in the rest of the file.  I can convert the whole file
in another changelist if you like.

[Dmitry Titov, 2016-08-09 23:46:33]

I think you've missed this one: 


https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:104: for
(auto iter = requests_.begin(); iter != requests_.end(); ++iter) {
I think requests_ is already a map from request_id, so it is enough to just walk
the request_ids and for each id find the request in the requests_...


Also:

https://codereview.chromium.org/2221323003/diff/60001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2221323003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:352: void
RequestQueueStoreSQL::PauseRequestsSync(
I am not sure the store must have methods like Pause/Resume etc. All of those
are just changing a single data field, 'state'. From Storage pov, it's just
changing state. SO there can be just one single method. This will also reduce
amount of boilerplate.

I would think that RequestQueue also does not have to keep these 2 methods. From
Queue perspective, it's also just a state change.

It looks like only RequestCoordinator has to have those methods, quickly
converting them to something like RequestQueue::ChangeRequestState(..) and
possibly updating the scheduler etc...

https://codereview.chromium.org/2221323003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:413: bool
RequestQueueStoreSQL::CheckDb(const UpdateCallback& callback) {
The answer to multiple CheckDB functions is to pass a bound callback, like this:
if (!CheckDb(base::Bind(callback, false))
  return;

that will make all parameters simply Closure, right?

[Pete Williamson, 2016-08-10 01:34:10]

https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
File components/offline_pages/background/request_queue_in_memory_store.cc
(right):

https://codereview.chromium.org/2221323003/diff/40001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:104: for
(auto iter = requests_.begin(); iter != requests_.end(); ++iter) {
On 2016/08/09 21:01:43, Dmitry Titov wrote:
> I think requests_ is already a map from request_id, so it is enough to just
walk
> the request_ids and for each id find the request in the requests_...

Done.

https://codereview.chromium.org/2221323003/diff/60001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2221323003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:352: void
RequestQueueStoreSQL::PauseRequestsSync(
On 2016/08/09 23:46:33, Dmitry Titov wrote:
> I am not sure the store must have methods like Pause/Resume etc. All of those
> are just changing a single data field, 'state'. From Storage pov, it's just
> changing state. SO there can be just one single method. This will also reduce
> amount of boilerplate.
> 
> I would think that RequestQueue also does not have to keep these 2 methods.
From
> Queue perspective, it's also just a state change.
> 
> It looks like only RequestCoordinator has to have those methods, quickly
> converting them to something like RequestQueue::ChangeRequestState(..) and
> possibly updating the scheduler etc...

Done.

https://codereview.chromium.org/2221323003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:413: bool
RequestQueueStoreSQL::CheckDb(const UpdateCallback& callback) {
On 2016/08/09 23:46:33, Dmitry Titov wrote:
> The answer to multiple CheckDB functions is to pass a bound callback, like
this:
> if (!CheckDb(base::Bind(callback, false))
>   return;
> 
> that will make all parameters simply Closure, right?

Done.

[fgorski, 2016-08-10 16:00:10]

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue.h (right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue.h:23: class SavePageRequest;
please remove now that you are adding the .h file.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue.h:89: void
ChangeRequestsState(const std::vector<int64_t>& request_ids,
Is the plan to add one of these for everything that we are going to be changing?
What other options did you consider, before we settle on this?

I see Dmitry made a similar comment on this patch before. Can we discuss this in
the office?

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_in_memory_store.cc
(right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:85:
requests_.begin(), requests_.end(),
I don't think lambda is necessary here, as you we are keying the map using
request_id.

See lines 47-51 for equivalent code without <algorithm>

For finding stuff by other fields this code makes sense, thou.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:97: count >
0 ? UpdateStatus::UPDATED : UpdateStatus::FAILED;
what if there were no items?
Makes me think that our UpdateCallback could be structured more like
DeleteCallback. Please add a TODO in request_queue_store.h for me.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_in_memory_store.h
(right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.h:26: 
nit: remove this space, as methods below are still part of the RQ implementation
block.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.h:37: 
nit: same here.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store.h (right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store.h:17: class
SavePageRequest;
remove, please.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:87:
statement.BindInt64(0, static_cast<long>(new_state));
please update the cast to type to int64_t.
This will ensure no weird compiler issues and will be self consistent with the
code to insert the status.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:401: if
(!db_.get()) {
CheckDB please.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.h (right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.h:37: 
nit: same comment for this and next empty line as before: please remove.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store_unittest.cc
(right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:396: //
Create requests and add them to the queue.
I don't think request 2 is added. Fix the comment or the code please.

[Pete Williamson, 2016-08-10 21:38:01]

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue.h (right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue.h:23: class SavePageRequest;
On 2016/08/10 16:00:09, fgorski wrote:
> please remove now that you are adding the .h file.

Done.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue.h:89: void
ChangeRequestsState(const std::vector<int64_t>& request_ids,
On 2016/08/10 16:00:09, fgorski wrote:
> Is the plan to add one of these for everything that we are going to be
changing?
> What other options did you consider, before we settle on this?
> 
> I see Dmitry made a similar comment on this patch before. Can we discuss this
in
> the office?

Sure, let's discuss this.  Leaving code as is for now, I'll change if we decide
to after our discussion.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_in_memory_store.cc
(right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:85:
requests_.begin(), requests_.end(),
On 2016/08/10 16:00:09, fgorski wrote:
> I don't think lambda is necessary here, as you we are keying the map using
> request_id.
> 
> See lines 47-51 for equivalent code without <algorithm>
> 
> For finding stuff by other fields this code makes sense, thou.

Ah, good catch, I totally forgot what the map was keyed on.  Fixed.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:97: count >
0 ? UpdateStatus::UPDATED : UpdateStatus::FAILED;
On 2016/08/10 16:00:09, fgorski wrote:
> what if there were no items?
> Makes me think that our UpdateCallback could be structured more like
> DeleteCallback. Please add a TODO in request_queue_store.h for me.

Done.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_in_memory_store.h
(right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.h:26: 
On 2016/08/10 16:00:09, fgorski wrote:
> nit: remove this space, as methods below are still part of the RQ
implementation
> block.

Done.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.h:37: 
On 2016/08/10 16:00:09, fgorski wrote:
> nit: same here.

Done.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store.h (right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store.h:17: class
SavePageRequest;
On 2016/08/10 16:00:09, fgorski wrote:
> remove, please.

Done.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:87:
statement.BindInt64(0, static_cast<long>(new_state));
On 2016/08/10 16:00:09, fgorski wrote:
> please update the cast to type to int64_t.
> This will ensure no weird compiler issues and will be self consistent with the
> code to insert the status.

Done.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:401: if
(!db_.get()) {
On 2016/08/10 16:00:09, fgorski wrote:
> CheckDB please.

This whole function will be removed in a changelist that I expect to go in
before this one.  I'll have less merge pain if I don't modify the function at
all.  OK to leave it for now?

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.h (right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.h:37: 
On 2016/08/10 16:00:09, fgorski wrote:
> nit: same comment for this and next empty line as before: please remove.

Done.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store_unittest.cc
(right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:396: //
Create requests and add them to the queue.
On 2016/08/10 16:00:09, fgorski wrote:
> I don't think request 2 is added. Fix the comment or the code please.

Done.

[fgorski, 2016-08-10 21:49:17]

lgtm mod a nit and discussion we need to have in the office.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:401: if
(!db_.get()) {
On 2016/08/10 21:38:00, Pete Williamson wrote:
> On 2016/08/10 16:00:09, fgorski wrote:
> > CheckDB please.
> 
> This whole function will be removed in a changelist that I expect to go in
> before this one.  I'll have less merge pain if I don't modify the function at
> all.  OK to leave it for now?

Acknowledged.

https://codereview.chromium.org/2221323003/diff/100001/components/offline_pag...
File components/offline_pages/background/request_queue_store_unittest.cc
(right):

https://codereview.chromium.org/2221323003/diff/100001/components/offline_pag...
components/offline_pages/background/request_queue_store_unittest.cc:396: //
Create requests and add them to the queue.
Drop the "s", just "request" it's cleaner.

[Pete Williamson, 2016-08-10 23:34:41]

This should address all FGorski issues, still waiting to see if Dimich has more
feedback.

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue.h (right):

https://codereview.chromium.org/2221323003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue.h:89: void
ChangeRequestsState(const std::vector<int64_t>& request_ids,
On 2016/08/10 21:38:00, Pete Williamson wrote:
> On 2016/08/10 16:00:09, fgorski wrote:
> > Is the plan to add one of these for everything that we are going to be
> changing?
> > What other options did you consider, before we settle on this?
> > 
> > I see Dmitry made a similar comment on this patch before. Can we discuss
this
> in
> > the office?
> 
> Sure, let's discuss this.  Leaving code as is for now, I'll change if we
decide
> to after our discussion.

After discussion, plan is to start updating the entire request instead of
individual fields of the item, so added a TODO here to change to the new way.

https://codereview.chromium.org/2221323003/diff/100001/components/offline_pag...
File components/offline_pages/background/request_queue_store_unittest.cc
(right):

https://codereview.chromium.org/2221323003/diff/100001/components/offline_pag...
components/offline_pages/background/request_queue_store_unittest.cc:396: //
Create requests and add them to the queue.
On 2016/08/10 21:49:16, fgorski wrote:
> Drop the "s", just "request" it's cleaner.

Done.

[Dmitry Titov, 2016-08-11 04:25:30]

lgtm with a nit and understanding that no new code should be written before
TODOs from the recent package of CLs are cleared up... Or it'll be more
difficult to do later.

[Dmitry Titov, 2016-08-11 04:25:54]

Oh, forgot to actually publish a nit:

https://codereview.chromium.org/2221323003/diff/120001/components/offline_pag...
File components/offline_pages/background/request_queue_store.h (right):

https://codereview.chromium.org/2221323003/diff/120001/components/offline_pag...
components/offline_pages/background/request_queue_store.h:25: // TODO(fgorski):
What if no items were updated?  Maybe this should work more
I think this is TODO(petewil) based on changing ChangeRequestsState to a
ChangeRequests. Then callback should probably come with a vector of pairs
<request_id_passed_in, result> with "result" being CHANGED/FAILED/NOT_FOUND or
something like that...

[Pete Williamson, 2016-08-12 00:06:46]

https://codereview.chromium.org/2221323003/diff/120001/components/offline_pag...
File components/offline_pages/background/request_queue_store.h (right):

https://codereview.chromium.org/2221323003/diff/120001/components/offline_pag...
components/offline_pages/background/request_queue_store.h:25: // TODO(fgorski):
What if no items were updated?  Maybe this should work more
On 2016/08/11 04:25:54, Dmitry Titov wrote:
> I think this is TODO(petewil) based on changing ChangeRequestsState to a
> ChangeRequests. Then callback should probably come with a vector of pairs
> <request_id_passed_in, result> with "result" being CHANGED/FAILED/NOT_FOUND or
> something like that...

Done.

[Pete Williamson, 2016-08-12 17:49:28]

Just some merge fixes with the other large patch that I submitted to change from
ClientId back to RequestId.

[Pete Williamson, 2016-08-12 17:49:35]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-08-12 17:49:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2221323003/#ps160001
(title: "Merges in the RequestId patch")

[commit-bot: I haz the power, 2016-08-12 17:50:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 18:04:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 18:04:02]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Pete Williamson, 2016-08-12 18:34:09]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-08-12 18:34:10]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2221323003/#ps180001
(title: "Fix unit test break introduced by merge")

[commit-bot: I haz the power, 2016-08-12 18:34:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 19:17:40]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-12 19:19:16]

Description was changed from

==========
Add an API to Pause and Resume background offlining requests.

The new API, PauseRequestsbyClientId and ResumeRequestsByClientId
is defined and implemented.  Tests are included.

It is basically parallel to the existing RemoveRequestByClientId,
and you can compare them when reviewing to speed up the review
process.

It is implemented all the way down into the request queue,
including both the in-memory queue and the SQL queue.

BUG=610521
==========

to

==========
Add an API to Pause and Resume background offlining requests.

The new API, PauseRequestsbyClientId and ResumeRequestsByClientId
is defined and implemented.  Tests are included.

It is basically parallel to the existing RemoveRequestByClientId,
and you can compare them when reviewing to speed up the review
process.

It is implemented all the way down into the request queue,
including both the in-memory queue and the SQL queue.

BUG=610521

Committed: https://crrev.com/48d69bf7b3937d03580c90e06767e0f909dab6be
Cr-Commit-Position: refs/heads/master@{#411726}
==========

[commit-bot: I haz the power, 2016-08-12 19:19:18]

Patchset 10 (id:??) landed as
https://crrev.com/48d69bf7b3937d03580c90e06767e0f909dab6be
Cr-Commit-Position: refs/heads/master@{#411726}