Repository navigation
Scale gcs metrics center write with task name - #444
Conversation
| // json files according to bot names or task names. Skia perf read all | ||
| // json files in the directory so one can use arbitrary names for those | ||
| // sharded json file names. | ||
| // Too many bots writing the metrics of a git revision into a single json |
There was a problem hiding this comment.
I'm for this change, but it doesn't make sense why infra runs into this. Is the results directory being cached between runs?
There was a problem hiding this comment.
I don't think the results are cached anywhere. Different tasks/bots randomly access the file, read and write back. It is strange why the issue pops up after switching logic from cocoon to test runner, but it does sound like lock contention.
Hopefully this solves the problem.
There was a problem hiding this comment.
I see the issue. The recipe implemented retry logic on the entire test runner. Thus,
-> recipe creates tmp results.json
-> test run 1: infra failure, writes results.json
-> test run 2: recipe retries, infra failure, results.json contention
-> test run 3: recipe retries last time, infra failure, results.json contention
If we're going to implement retries in the recipe, it needs to handle this cleanup. Or we can delete the file if it exists instead. However, it's intentional we fail as it indicates a dirty tree.
Preferably, we'd implement retries in the test runner and not the recipe.
There was a problem hiding this comment.
This is a gcs lock on values.json, rather than a local lock on the results json from the test runner.
The retries are already implemented in test runner: https://cs.opensource.google/flutter/flutter/+/master:dev/devicelab/lib/framework/runner.dart;l=47, so it should not cause local json file read/write contention since we delete it first if existing.
There was a problem hiding this comment.
Ah perfect! So the issue is we're uploading values.json directly to GCS? How does that get to skia perf?
There was a problem hiding this comment.
We are reading values.json and write it back on the fly, and that's why we have the gcs lock.
With this PR, we are dividing the single json file in gcs to be dependent on task name, so that each task has its own json file and there should not be any contention.
Skia perf reads data directly from GCS bucket.
There was a problem hiding this comment.
So to verify my understanding, the test runner pushes to GCS, and skia perf pulls from GCS? Then the contention is because we're uploading ~100 results to the same location, correct?
| final String hour = commitUtcTime.hour.toString().padLeft(2, '0'); | ||
| final String dateComponents = '${commitUtcTime.year}/$month/$day/$hour'; | ||
| return '$topComponent/$dateComponents/$revision/values.json'; | ||
| return '$topComponent/$dateComponents/$revision/${taskName}_values.json'; |
There was a problem hiding this comment.
Does task name matter? Could we just generate a random id
There was a problem hiding this comment.
Yeah, it would be more meaningful to include the task name so we can track different values for a specific task.
| Future<void> update(List<MetricPoint> points, DateTime commitTime) async { | ||
| await _skiaPerfDestination.update(points, commitTime); | ||
| Future<void> update( | ||
| List<MetricPoint> points, DateTime commitTime, String taskName) async { |
There was a problem hiding this comment.
nit: metric name is more apt in this context
There was a problem hiding this comment.
Task name is more preferred, as a task contains multiple different metrics and we want to separate values based on different tasks.
CaseyHillers
left a comment
CaseyHillers
left a comment
There was a problem hiding this comment.
LGTM - Thanks for the explainations!
3 participants
This is to scale gcs writes by appending task name to objects. This way there will not be contention on write lock.
Issue: flutter/flutter#88977