Bug 194961 – [ews-app] Remove BuilderMapping table

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

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

[Aakash Jain]

Created attachment 362767 [details]
Proposed patch

[Jonathan Bedard]

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'

[Aakash Jain]

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

[Dean Johnson]

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.

[Aakash Jain]

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

[Aakash Jain]

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

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.

[Aakash Jain]

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

OK. Does that mean avoid "keep multiple versions of the mapping" is not the major benefit?

[Aakash Jain]

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

[WebKit Commit Bot]

Comment on attachment 362909 [details]
Proposed patch

Clearing flags on attachment: 362909

Committed r242065: <https://trac.webkit.org/changeset/242065>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48387573>