Bug 170932

Summary: Introduce 'commit_group' concept to be able to distinguish between commits with same revision but from different groups.
Product: WebKit Reporter: dewei_zhu
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: dewei_zhu, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description dewei_zhu 2017-04-17 23:25:30 PDT
Introduce 'commit_group' concept to be able to distinguish between commits with same revision but from different groups.
Comment 1 dewei_zhu 2017-04-17 23:33:28 PDT
Created attachment 307351 [details]
Patch
Comment 2 Ryosuke Niwa 2017-04-18 00:12:46 PDT
Comment on attachment 307351 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:12
> +            CREATE TABLE commit_groups (
> +                commit_group_id serial PRIMARY KEY,
> +                commit_group_name varchar(64),
> +                CONSTRAINT commit_group_name_must_be_unique UNIQUE(commit_group_name));

I think we should just add a new column to commits which is varchar(64) instead of adding a new table.

> Websites/perf.webkit.org/ChangeLog:23
> +            

Nit: Whitespace.

> Websites/perf.webkit.org/public/api/commits.php:29
> +        $group = array_get($_GET, 'group');
> +        $commits = $fetcher->fetch_between($repository_id, $preceding_revision, $last_revision, $group, $keyword);

We need to make the JSON API's parameters consistent.
Here, group is assumed to be an ID.

> Websites/perf.webkit.org/public/api/commits.php:47
> +        $group_name = array_get($_GET, 'group');
> +        if ($group_name)
> +            $commits = $fetcher->fetch_last_reported_for_commit_group($repository_id, $group_name);

And it's assumed to be a string.

> Websites/perf.webkit.org/public/api/report-commits.php:42
> +
> +

Nit: two spaces.

> Websites/perf.webkit.org/public/api/report-commits.php:104
> -    $inserted_commit_id = $db->update_or_insert_row('commits', 'commit', array('repository' => $repository_id, 'revision' => $data['revision']), $data);
> +    $inserted_commit_id = $db->update_or_insert_row('commits', 'commit', array('repository' => $repository_id, 'revision' => $data['revision'], 'group' => $data['group']), $data);

This is problematic because the existing database rows would have group being set to null.
So this results in all macOS & iOS builds being inserted again.
Also, /api/report doesn't set group name so this might end up creating a new row as well.
Comment 3 Ryosuke Niwa 2017-05-09 19:08:32 PDT
Didn't we conclude that this approach was bad?