[webkit-reviews] review denied: [Bug 190188] Add retry for test groups with failed build requests. : [Attachment 351353] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 190188: Add retry for test groups with failed build requests.
https://bugs.webkit.org/show_bug.cgi?id=190188

Attachment 351353: Patch

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




--- 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.