[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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #351353|review?                     |review-
              Flags|                            |

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

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

> Websites/perf.webkit.org/ChangeLog:18
> +                ADD COLUMN testgroup_expected_successful_build_requests integer DEFAULT NULL,
> +                ADD COLUMN testgroup_may_need_additional_build_requests boolean DEFAULT FALSE;

These names are too long. How about just: testgroup_iteration_count & testgroup_may_need_more_requests.

> Websites/perf.webkit.org/public/api/build-requests.php:77
> +    if (count($test_groups_may_need_additional_build_requests))
> +        $db->query('UPDATE analysis_test_groups SET testgroup_may_need_additional_build_requests = TRUE WHERE testgroup_id = ANY($1)',
> +            array('{' . implode(',', array_keys($test_groups_may_need_additional_build_requests)) . '}'));

I don't think we need to coerce queries like this.
Just issue a SQL query per build request.
We can certainly have a map which stores the set of test groups we've already set this flag.

> Websites/perf.webkit.org/public/api/test-groups.php:25
> -    } elseif ($path[0] == 'ready-for-notification') {
> +    } elseif ($path[0] == 'ready-for-further-processing') {

I think we should just add a new API surface instead of replacing an existing one.

> Websites/perf.webkit.org/public/privileged-api/update-test-group.php:19
> +        if (array_key_exists('name', $data) || array_key_exists('hidden', $data) || array_key_exists('mayNeedAdditionalBuildRequests', $data))
> +            exit_with_error('TooManyArgumentsProvided');

Why don't we add a new dedicated API instead?
I think that'd be cleaner than re-using this API and having restrictions like these.

> Websites/perf.webkit.org/public/privileged-api/update-test-group.php:28
>          $values['hidden'] = Database::to_database_boolean($data['hidden']);
> +        $values['may_need_additional_build_requests'] = FALSE;

I think we probably want another check at where we create build requests
that we haven't recently canceled this test group.

> Websites/perf.webkit.org/public/privileged-api/update-test-group.php:58
> +        for ($i = 0; $i < $additional_build_request_count; $i++) {
> +            $processed_commit_set = array();

Why don't we split the part to find the list of commit sets in this group into a separate helper function?

> Websites/perf.webkit.org/public/v3/models/test-group.js:14
> +        this._mayNeedAdditionalBuildRequests = object.mayNeedAdditionalBuildRequests;
> +        this._expectedSuccessfulBuildRequests = object.expectedSuccessfulBuildRequests;

Let's also shorten these names.

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

Just call it "count".

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

No need to say "flag".

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:67
> +    addMockData: function (db, statusList, needs_notification=true)

Use camelCase.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:82
> +            db.insert('analysis_test_groups', {id: 600, task: 500, name: 'some test group', expected_successful_build_requests: 4, needs_notification}),

Ditto here and elsewhere in this test.

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

Maybe even 3 might be okay.

> Websites/perf.webkit.org/tools/run-analysis.js:67
> +async function processTestGroupsNeedAdditionalBuildRequests(testGroups, maximumRetryRatio)

I don't think we want to use vague terms like "processing".
Let's just say createAdditionalBuildRequestsForTestGroupsWithFailedRequests or something.
We can use a verbose name here.

> Websites/perf.webkit.org/tools/run-analysis.js:80
> +        let maxMissingSuccessfulBuildRequestCount = 0;
> +        for (const commitSet of this.requestedCommitSet()) {
> +            const buildRequests = testGroup.requestsForCommitSet(commitSet);
> +            maxMissingSuccessfulBuildRequestCount = Math.max(maxMissingSuccessfulBuildRequestCount, buildRequests.filter((buildRequest) => !buildRequest.hasCompleted()).length);
> +        }

I don't think this logic is quite right.
buildRequest.hasCompleted() returns false for pending & canceled requests.
r- because of this.

In fact, we probably want to make sure this code handles the case when some request are still pending, scheduled, or canceled.

-- 
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/20181002/f8f91cd1/attachment-0001.html>