[webkit-reviews] review denied: [Bug 190188] Add retry for test groups with failed build requests. : [Attachment 351353] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Oct 2 01:34:24 PDT 2018
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.
More information about the webkit-reviews
mailing list