tools/android/loading Support "report" tasks in the Clovis frontend

tools/android/loading Support "report" tasks in the Clovis frontend

Committed: https://crrev.com/48ddb18aa346a2206e339991de689c1465f600e2
Cr-Commit-Position: refs/heads/master@{#394085}

[droger, 2016-05-10 14:34:07]

droger@chromium.org changed reviewers:
+ blundell@chromium.org, lizeb@chromium.org

[blundell, 2016-05-11 10:35:39]

https://codereview.chromium.org/1962283002/diff/1/tools/android/loading/cloud...
File tools/android/loading/cloud/frontend/README.md (right):

https://codereview.chromium.org/1962283002/diff/1/tools/android/loading/cloud...
tools/android/loading/cloud/frontend/README.md:42: -   `urls` (list of strings):
the list of URLs to process.
I would think we'll also want a mode where you just point to a trace directory
and it processes all the URLs underneath. WDYT?

[droger, 2016-05-11 11:16:27]

https://codereview.chromium.org/1962283002/diff/1/tools/android/loading/cloud...
File tools/android/loading/cloud/frontend/README.md (right):

https://codereview.chromium.org/1962283002/diff/1/tools/android/loading/cloud...
tools/android/loading/cloud/frontend/README.md:42: -   `urls` (list of strings):
the list of URLs to process.
On 2016/05/11 10:35:39, blundell wrote:
> I would think we'll also want a mode where you just point to a trace directory
> and it processes all the URLs underneath. WDYT?

Very good point.
This was my initial idea too, but then I decided to go by URLs, because it
seemed much more difficult to implement (see below).

If we do it per directory, then it's also not clear whether we would want that
in addition to the URL mode. Maybe the directory mode would be enough.


The reason why I chose URLs instead of directory is because it is much simpler
to split the work into small tasks (splitting a list of URLs is easy).

If I wanted to take a directory as input, I would need either to:

-reconstruct the list of URLs in the frontend, which means that the frontend
would need to load and call into things LoadingTraceDatabase, which seems a bit
complex (I liked the idea of keeping the frontend relatively dumb, and also
would require some refactoring of the directory structure so that appengine can
reference this code).

- or split the work across another dimension. The most obvious being to find all
the loading trace database files (just by doing a prefix match, and without
loading them), and create one task per file. The downside of this approach is
that these tasks will be rather large.

We can definitely discuss more about this.

[blundell, 2016-05-12 08:58:00]

On 2016/05/11 11:16:27, droger wrote:
>
https://codereview.chromium.org/1962283002/diff/1/tools/android/loading/cloud...
> File tools/android/loading/cloud/frontend/README.md (right):
> 
>
https://codereview.chromium.org/1962283002/diff/1/tools/android/loading/cloud...
> tools/android/loading/cloud/frontend/README.md:42: -   `urls` (list of
strings):
> the list of URLs to process.
> On 2016/05/11 10:35:39, blundell wrote:
> > I would think we'll also want a mode where you just point to a trace
directory
> > and it processes all the URLs underneath. WDYT?
> 
> Very good point.
> This was my initial idea too, but then I decided to go by URLs, because it
> seemed much more difficult to implement (see below).
> 
> If we do it per directory, then it's also not clear whether we would want that
> in addition to the URL mode. Maybe the directory mode would be enough.
> 
> 
> The reason why I chose URLs instead of directory is because it is much simpler
> to split the work into small tasks (splitting a list of URLs is easy).
> 
> If I wanted to take a directory as input, I would need either to:
> 
> -reconstruct the list of URLs in the frontend, which means that the frontend
> would need to load and call into things LoadingTraceDatabase, which seems a
bit
> complex (I liked the idea of keeping the frontend relatively dumb, and also
> would require some refactoring of the directory structure so that appengine
can
> reference this code).

As discussed offline, let's try out this approach. It seems like it shouldn't
add very much intelligence to the frontend.

> 
> - or split the work across another dimension. The most obvious being to find
all
> the loading trace database files (just by doing a prefix match, and without
> loading them), and create one task per file. The downside of this approach is
> that these tasks will be rather large.
> 
> We can definitely discuss more about this.

[droger, 2016-05-13 10:10:22]

On 2016/05/12 08:58:00, blundell wrote:
> As discussed offline, let's try out this approach. It seems like it shouldn't
> add very much intelligence to the frontend.

Done.

[blundell, 2016-05-13 11:19:55]

lgtm

https://codereview.chromium.org/1962283002/diff/80001/tools/android/loading/c...
File tools/android/loading/cloud/frontend/clovis_frontend.py (right):

https://codereview.chromium.org/1962283002/diff/80001/tools/android/loading/c...
tools/android/loading/cloud/frontend/clovis_frontend.py:164: return
SplitTask(task, 'urls')
I would just fold SplitTask() into this function:

- Have a variable |split_key| that gets set appropriately in the various cases.

- Inline the body of SplitTask() starting at line 175.

https://codereview.chromium.org/1962283002/diff/80001/tools/android/loading/c...
tools/android/loading/cloud/frontend/clovis_frontend.py:168:
clovis_logger.error('Missing storage bucket fot report task.')
nit: for

https://codereview.chromium.org/1962283002/diff/80001/tools/android/loading/c...
tools/android/loading/cloud/frontend/clovis_frontend.py:171: if not traces:
In general, your functions could use some blank lines as visual punctuation.

https://codereview.chromium.org/1962283002/diff/80001/tools/android/loading/c...
tools/android/loading/cloud/frontend/clovis_frontend.py:243: table_id =
''.join(c for c in task_tag if c.isalnum() or c == '_')
Maybe this should be defined in a function in common/?

https://codereview.chromium.org/1962283002/diff/80001/tools/android/loading/c...
tools/android/loading/cloud/frontend/clovis_frontend.py:284: key(str): The key
in the action parameters that is used as the dimension of
same comment as the other one re: type information.

[droger, 2016-05-13 14:39:47]

Patchset #6 (id:100001) has been deleted

[Benoit L, 2016-05-13 15:23:41]

lgtm, thanks.

https://codereview.chromium.org/1962283002/diff/120001/tools/android/loading/...
File tools/android/loading/cloud/frontend/clovis_frontend.py (right):

https://codereview.chromium.org/1962283002/diff/120001/tools/android/loading/...
tools/android/loading/cloud/frontend/clovis_frontend.py:163: # For report trask,
need to find the traces first.
nit: task.

https://codereview.chromium.org/1962283002/diff/120001/tools/android/loading/...
tools/android/loading/cloud/frontend/clovis_frontend.py:176:
split_params_for_action = {'trace': ('urls',1), 'report': ('traces',5)}
tiny, tiny nit: one space after a comma.

[droger, 2016-05-13 15:44:17]

Thank you both.

Done except one comment that I would prefer to address in a follow up CL.

https://codereview.chromium.org/1962283002/diff/80001/tools/android/loading/c...
File tools/android/loading/cloud/frontend/clovis_frontend.py (right):

https://codereview.chromium.org/1962283002/diff/80001/tools/android/loading/c...
tools/android/loading/cloud/frontend/clovis_frontend.py:243: table_id =
''.join(c for c in task_tag if c.isalnum() or c == '_')
On 2016/05/13 11:19:55, blundell wrote:
> Maybe this should be defined in a function in common/?

Good point.
I'm doing a separate CL for this though, to keep things small in this CL.

[droger, 2016-05-17 07:57:01]

The CQ bit was checked by droger@chromium.org

[droger, 2016-05-17 07:57:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/1962283002/#ps140001
(title: "lizeb comments")

[commit-bot: I haz the power, 2016-05-17 07:57:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962283002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962283002/140001

[commit-bot: I haz the power, 2016-05-17 08:48:30]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-05-17 08:50:39]

Description was changed from

==========
tools/android/loading Support "report" tasks in the Clovis frontend
==========

to

==========
tools/android/loading Support "report" tasks in the Clovis frontend

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

[commit-bot: I haz the power, 2016-05-17 08:50:40]

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