tools/android/loading Loading trace database improvements

tools/android/loading Loading trace database improvements

The loading trace database is now reloaded from the cloud when the
worker starts, which prevents losing data when the worker restarts after
a failure.

There is also now one trace database per worker, which solves the issues
of concurrent access to the database.

The CL also adds supports for the 'pushStart' and 'pushEnd'
events that were added by CL:
https://codereview.chromium.org/1828203005/

Committed: https://crrev.com/4a50ba25fd9177163031cfc403e2d7e4bf036d99
Cr-Commit-Position: refs/heads/master@{#388530}

[droger, 2016-04-20 14:39:12]

Patchset #1 (id:1) has been deleted

[droger, 2016-04-20 14:39:42]

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

[Benoit L, 2016-04-20 15:02:22]

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/c...
File tools/android/loading/cloud/backend/worker.py (right):

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/c...
tools/android/loading/cloud/backend/worker.py:53: self._traces_dir =
self._base_path_in_bucket + 'traces/'
Generally speaking, I prefer to manipulate paths with os.path.join(), this
avoids having to care about whether all parts are neatly separated. It would
only blow up if the script is running on Windows and you want '/' as a
separator.

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/c...
tools/android/loading/cloud/backend/worker.py:55: 'trace_database_filename',
'trace_database.json')
Is the trace dir per worker?
If not, then the trace DB is not per-worker by default, or am I missing
something?

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/c...
tools/android/loading/cloud/backend/worker.py:102: trace_database_string =
self._google_storage_accessor.DownloadAsString(
What about:

trace_db_str = self._google_storage_accessor...() or '{}'
self._trace_database = LoadingTraceDatabase(json.loads(trace_db_str))

Tangentially related: looks like we upload / download JSON quite often. Why not
add that to GoogleStorageAccessor?

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/c...
tools/android/loading/cloud/backend/worker.py:113: self._logger.info('Uploading
trace database')
tiny nit: Importing is "warning", uploading "info".

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/r...
File tools/android/loading/request_track.py (right):

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/r...
tools/android/loading/request_track.py:28: 'loadingFinished':
'loading_finished', 'pushStart' : 'push_start',
Can you list this in the CL description as well?

[droger, 2016-04-20 15:44:13]

Description was changed from

==========
tools/android/loading Loading trace database improvements

The loading trace database is now reloaded from the cloud when the
worker starts, which prevents losing data when the worker restarts after
a failure.

There is also now one trace database per worker, which solves the issues
of concurrent access to the database.
==========

to

==========
tools/android/loading Loading trace database improvements

The loading trace database is now reloaded from the cloud when the
worker starts, which prevents losing data when the worker restarts after
a failure.

There is also now one trace database per worker, which solves the issues
of concurrent access to the database.

The CL also adds supports for the 'pushStart' and 'pushEnd'
events that were added by CL:
https://codereview.chromium.org/1828203005/
==========

[droger, 2016-04-20 15:58:17]

Thanks.

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/c...
File tools/android/loading/cloud/backend/worker.py (right):

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/c...
tools/android/loading/cloud/backend/worker.py:53: self._traces_dir =
self._base_path_in_bucket + 'traces/'
On 2016/04/20 15:02:22, Benoit L wrote:
> Generally speaking, I prefer to manipulate paths with os.path.join(), this
> avoids having to care about whether all parts are neatly separated. It would
> only blow up if the script is running on Windows and you want '/' as a
> separator.

Done.

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/c...
tools/android/loading/cloud/backend/worker.py:55: 'trace_database_filename',
'trace_database.json')
On 2016/04/20 15:02:22, Benoit L wrote:
> Is the trace dir per worker?
> If not, then the trace DB is not per-worker by default, or am I missing
> something?

It is intended that all worker write to the same dir, but different files.
If you don't specify a 'trace_database_filename', then yes all workers would
write to the same file, which would not work.

I can make this key required instead of optional, but I don't think it would
solve the problem, since nothing is preventing several worker to use the same
file.

I'm adding a comment in the README to clarify.

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/c...
tools/android/loading/cloud/backend/worker.py:102: trace_database_string =
self._google_storage_accessor.DownloadAsString(
On 2016/04/20 15:02:22, Benoit L wrote:
> What about:
> 
> trace_db_str = self._google_storage_accessor...() or '{}'
> self._trace_database = LoadingTraceDatabase(json.loads(trace_db_str))

Done.

> 
> Tangentially related: looks like we upload / download JSON quite often. Why
not
> add that to GoogleStorageAccessor?

For me, what would make the most sense would be that the
LoadingTraceDatabase.ToJson() and FromJson() functions actually return JSON
(strings) instead of dictionaries.

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/c...
tools/android/loading/cloud/backend/worker.py:113: self._logger.info('Uploading
trace database')
On 2016/04/20 15:02:22, Benoit L wrote:
> tiny nit: Importing is "warning", uploading "info".

Done.

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/r...
File tools/android/loading/request_track.py (right):

https://codereview.chromium.org/1908483002/diff/20001/tools/android/loading/r...
tools/android/loading/request_track.py:28: 'loadingFinished':
'loading_finished', 'pushStart' : 'push_start',
On 2016/04/20 15:02:22, Benoit L wrote:
> Can you list this in the CL description as well?

Done.

[Benoit L, 2016-04-20 16:07:45]

Thanks!
lgtm

[droger, 2016-04-20 16:23:08]

The CQ bit was checked by droger@chromium.org

[droger, 2016-04-20 16:23:08]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/1908483002/#ps80001
(title: "Minor cleanup")

[commit-bot: I haz the power, 2016-04-20 16:23:33]

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

[commit-bot: I haz the power, 2016-04-20 16:28:39]

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

[commit-bot: I haz the power, 2016-04-20 16:28:41]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[droger, 2016-04-20 17:45:16]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2016-04-20 17:45:36]

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

[commit-bot: I haz the power, 2016-04-20 17:52:19]

Description was changed from

==========
tools/android/loading Loading trace database improvements

The loading trace database is now reloaded from the cloud when the
worker starts, which prevents losing data when the worker restarts after
a failure.

There is also now one trace database per worker, which solves the issues
of concurrent access to the database.

The CL also adds supports for the 'pushStart' and 'pushEnd'
events that were added by CL:
https://codereview.chromium.org/1828203005/
==========

to

==========
tools/android/loading Loading trace database improvements

The loading trace database is now reloaded from the cloud when the
worker starts, which prevents losing data when the worker restarts after
a failure.

There is also now one trace database per worker, which solves the issues
of concurrent access to the database.

The CL also adds supports for the 'pushStart' and 'pushEnd'
events that were added by CL:
https://codereview.chromium.org/1828203005/
==========

[commit-bot: I haz the power, 2016-04-20 17:52:20]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-04-22 19:24:43]

Description was changed from

==========
tools/android/loading Loading trace database improvements

The loading trace database is now reloaded from the cloud when the
worker starts, which prevents losing data when the worker restarts after
a failure.

There is also now one trace database per worker, which solves the issues
of concurrent access to the database.

The CL also adds supports for the 'pushStart' and 'pushEnd'
events that were added by CL:
https://codereview.chromium.org/1828203005/
==========

to

==========
tools/android/loading Loading trace database improvements

The loading trace database is now reloaded from the cloud when the
worker starts, which prevents losing data when the worker restarts after
a failure.

There is also now one trace database per worker, which solves the issues
of concurrent access to the database.

The CL also adds supports for the 'pushStart' and 'pushEnd'
events that were added by CL:
https://codereview.chromium.org/1828203005/

Committed: https://crrev.com/4a50ba25fd9177163031cfc403e2d7e4bf036d99
Cr-Commit-Position: refs/heads/master@{#388530}
==========

[commit-bot: I haz the power, 2016-04-22 19:24:45]

Patchset 4 (id:??) landed as
https://crrev.com/4a50ba25fd9177163031cfc403e2d7e4bf036d99
Cr-Commit-Position: refs/heads/master@{#388530}