[Webkit-unassigned] [Bug 190188] Add retry for test groups with failed build requests.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=190188

--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 351487
  --> https://bugs.webkit.org/attachment.cgi?id=351487
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351487&action=review

> Websites/perf.webkit.org/public/api/build-requests.php:70
> +            $db->update_row('analysis_test_groups', 'testgroup', array('id' => $test_group_id), array('may_need_more_requests' => TRUE));
> +            $test_groups_may_need_more_requests[$request_row['request_group']] = TRUE;

Let's move this code out of the if-else to share the code between two cases.

> Websites/perf.webkit.org/public/api/build-requests.php:80
> +            if ($status != 'failed' || array_key_exists($test_group_id, $test_groups_may_need_more_requests))
> +                continue;

And keep this continue when $status != 'failed'

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:10
> +    $test_group_id = array_get($data, 'group');

We need to bail out if the test group had been marked as "hidden" here.

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:19
> +
> +
> +

Extra blank lines.

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:23
> +    if ($current_order < 0)
> +        exit_with_error('NoBuildRequestOfTestType');

I think we should return NoTestingBuildRequests.

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:29
> +    for ($i = 0; $i < $additional_build_request_count; $i++) {
> +        $processed_commit_set = array();
> +        foreach ($existing_build_requests as $build_request) {

I think we need to split this into two separate loops:
The first one which finds an ordered list of commit sets (1),
and the second loop which generates N build requests for each commit set we found in (1).
Otherwise, this algorithm is really O(N^2).

$commit_sets = array();
$processed_commit_set = array();
foreach ($existing_build_requests as $build_request) {
    if ($build_request['request_order'] < 0)
        continue;
    $requested_commit_set = $build_request['request_commit_set'];
    if (array_key_exists($requested_commit_set, $processed_commit_set))
        continue;
    $processed_commit_set[$requested_commit_set] = &$build_request;
    array_push($commit_sets, $requested_commit_set);
}
for ($i = 0; $i < $additional_build_request_count; $i++) {
    foreach ($commit_sets as $commit_set) {...}
}

> Websites/perf.webkit.org/public/v3/models/test-group.js:231
> +    async addExtraBuildRequests(count)

This should be called addMoreBuildRequests

> Websites/perf.webkit.org/public/v3/models/test-group.js:239
> +    async clearNeedAdditionalBuildRequests()

This should be called clearMayNeedMoreBuildRequests

> Websites/perf.webkit.org/public/v3/models/test-group.js:297
> +    static fetchAllMayNeedMoreRequests()

fetchAllThatMayNeedMoreRequests / fetchAllWhichMayNeedMoreRequests?

> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:7
> +        if (testGroup.isHidden()) {

This early bail out is useless because we filter the results in API anyway.

Also we need to have an API test for this.

> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:18
> +            hasCompletedBuildRequestForEachCommitSet &= (completedBuildRequestCount > 0);

I think this logic doesn't work when one side all failed.

> Websites/perf.webkit.org/tools/js/retry-failed-build-requests.js:29
> +        await testGroup.clearNeedAdditionalBuildRequests();

I think it's not sound to clear the flag in a separate API call when when we're creating more build requests.
In fact, clearing the flag may is also racy but let's not worry about that for now.
We should, however, clear the flag automatically in /add-build-requests API.

> Websites/perf.webkit.org/tools/run-analysis.js:20
> +        {name: '--max-retry-ratio', type: parseFloat, default: 3},

We should probably call this a factor since it's not really a ratio.

> Websites/perf.webkit.org/unit-tests/retry-failed-build-requests-tests.js:144
> +    it('should add 1 more build request when one failed build request found', async () => {

This is rather confusing. Just say "should add one more build request when one of the existing requests failed".

> Websites/perf.webkit.org/unit-tests/retry-failed-build-requests-tests.js:145
> +        const data = sampleTestGroup(false, 3, true, false, ["completed", "completed", "completed", "completed", "completed", "failed"]);

Please use a dictionary to pass options to sampleTestGroup.
e.g. sampleTestGroup({needsNotification: true, iterationCount: 3, mayNeedMoreRequests: true, hidden: false}, [~])

> Websites/perf.webkit.org/unit-tests/retry-failed-build-requests-tests.js:174
> +    it('should not schedule more build requests when no completed build request for a commit set', async () => {

should read "when all requests for a commit set had failed".

> Websites/perf.webkit.org/unit-tests/retry-failed-build-requests-tests.js:192
> +    it('should not schedule more build requests when no completed build request is hidden', async () => {

"should not schedule more build requests when the test group is hidden"?

> Websites/perf.webkit.org/unit-tests/retry-failed-build-requests-tests.js:203
> +    it('should not schedule more build request while exceeding maximum retry', async () => {

when* we've already hit the maximum retry count.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181003/cd16f4d5/attachment-0001.html>