Bug 170932 - Introduce 'commit_group' concept to be able to distinguish between commits with same revision but from different groups.
Summary: Introduce 'commit_group' concept to be able to distinguish between commits wi...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-17 23:25 PDT by dewei_zhu
Modified: 2018-07-02 16:56 PDT (History)
2 users (show)

See Also:


Attachments
Patch (62.54 KB, patch)
2017-04-17 23:33 PDT, dewei_zhu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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?