Bug 194961 - [ews-app] Remove BuilderMapping table
Summary: [ews-app] Remove BuilderMapping table
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-22 14:59 PST by Aakash Jain
Modified: 2019-02-26 07:01 PST (History)
9 users (show)

See Also:


Attachments
Proposed patch (7.34 KB, patch)
2019-02-22 15:02 PST, Aakash Jain
no flags Details | Formatted Diff | Diff
Proposed patch (7.42 KB, patch)
2019-02-25 10:17 PST, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2019-02-22 14:59:40 PST
BuilderMapping table add a lot of complexity and maintenance overhead without much value addition (the main benefit being space saving, however calculations shows the space usage for build table is anyways extremely low). The BuilderMapping needs to be kept in-sync with Buildbot. Also if Buildbot machine is formatted for any reason, the mapping will reset and we need to keep multiple versions of the mapping (otherwise bubbles will show incorrect names).
Comment 1 Aakash Jain 2019-02-22 15:02:43 PST
Created attachment 362767 [details]
Proposed patch
Comment 2 Jonathan Bedard 2019-02-25 09:30:23 PST
Comment on attachment 362767 [details]
Proposed patch

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

I'd like to seem a bit more descriptive changelog, but other than that, looks good to me.

> Tools/ChangeLog:7
> +

Can we add a bit more of a description here? Something along the lines of 'Store builder information with build, instead of referencing builder'
Comment 3 Aakash Jain 2019-02-25 10:17:56 PST
Created attachment 362909 [details]
Proposed patch

> Can we add a bit more of a description here? Something along the lines of 'Store builder information with build, instead of referencing builder'
Done.

> I'd like to seem a bit more descriptive changelog, but other than that, looks good to me.
Thanks
Comment 4 Dean Johnson 2019-02-25 10:48:10 PST
Comment on attachment 362909 [details]
Proposed patch

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

LGTM overall.

> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:40
> +    builder_display_name = models.TextField()

Is it possible for this to be calculated at display time instead of stored? We could make this an @property if so, which would be cleaner.
Comment 5 Aakash Jain 2019-02-25 10:53:43 PST
Comment on attachment 362909 [details]
Proposed patch

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

>> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:40
>> +    builder_display_name = models.TextField()
> 
> Is it possible for this to be calculated at display time instead of stored? We could make this an @property if so, which would be cleaner.

Nope, there isn't a direct logic to convert builder full name to display name. We store both names in config.json (e.g.: https://ews-build.webkit-uat.org/config.json see 'name' and 'shortname').
Comment 6 dewei_zhu 2019-02-25 15:56:17 PST
Comment on attachment 362909 [details]
Proposed patch

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

>>> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:40
>>> +    builder_name = models.TextField()
>>> +    builder_display_name = models.TextField()
>> 
>> Is it possible for this to be calculated at display time instead of stored? We could make this an @property if so, which would be cleaner.
> 
> Nope, there isn't a direct logic to convert builder full name to display name. We store both names in config.json (e.g.: https://ews-build.webkit-uat.org/config.json see 'name' and 'shortname').

1. Assuming there hasn't been any running production instance. If there is, it's worth to mention a migration SQL for an existing instance.
2. Should the name/display_name be unique in the table? If not, the UI would be quite confusing assuming the display name is the one the shows in the UI.
Comment 7 Aakash Jain 2019-02-25 16:01:14 PST
> 1. Assuming there hasn't been any running production instance.
Nope, nothing in production currently.

> 2. Should the name/display_name be unique in the table? If not, the UI would be quite confusing assuming the display name is the one the shows in the UI.
build table contains all the builds across buildbot. So the display_name can't be unique in this table, since there will be multiple builds on any builder. To find builds for a given patch, we will need to query this table.
Comment 8 dewei_zhu 2019-02-25 16:16:56 PST
I see.
Correct me if I have any misunderstanding of this patch, the builder name or display name is actually a snapshot of the builder by the time the build is created. If so, calling it builder_name_snapshot and display_name_snapshot seems more accurate to me.
Comment 9 Aakash Jain 2019-02-25 17:15:30 PST
(In reply to dewei_zhu from comment #8)
> I see.
> Correct me if I have any misunderstanding of this patch, the builder name or display name is actually a snapshot of the builder by the time the build is created.
Yes (this data is sent by buildbot itself). But the builder name of an existing builder doesn't change frequently (we do add/remove builders, but rarely rename existing builder, even renaming a builder is equivalent to creating a new builder in buildbot 0.9+). Changing display name should be very rare.

>  If so, calling it builder_name_snapshot and display_name_snapshot seems more accurate to me.
So, I don't see a need to append snapshot to these field.
Comment 10 dewei_zhu 2019-02-25 17:30:35 PST
OK. Does that mean avoid "keep multiple versions of the mapping" is not the major benefit?
Comment 11 Aakash Jain 2019-02-25 18:06:17 PST
(In reply to dewei_zhu from comment #10)
> OK. Does that mean avoid "keep multiple versions of the mapping" is not the major benefit?
That's one of the benefit. Just to be clear, multiple versions of mappings would have been required for the scenario when "Buildbot machine is formatted". Such an event (data loss on Buildbot machine) would be rare, maybe once in few years. However, the logic would have been needed to be written to handle that scenario.
Comment 12 WebKit Commit Bot 2019-02-25 18:52:39 PST
Comment on attachment 362909 [details]
Proposed patch

Clearing flags on attachment: 362909

Committed r242065: <https://trac.webkit.org/changeset/242065>
Comment 13 WebKit Commit Bot 2019-02-25 18:52:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-02-25 18:53:26 PST
<rdar://problem/48387573>