Summary: | [ews-app] Remove BuilderMapping table | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aakash Jain <aakash_jain> | ||||||
Component: | Tools / Tests | Assignee: | Aakash Jain <aakash_jain> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aakash_jain, ap, commit-queue, dean_johnson, dewei_zhu, jbedard, lforschler, slewis, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=195047 https://bugs.webkit.org/show_bug.cgi?id=195045 |
||||||||
Attachments: |
|
Description
Aakash Jain
2019-02-22 14:59:40 PST
Created attachment 362767 [details]
Proposed patch
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' 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 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 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. > 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. (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? (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 on attachment 362909 [details] Proposed patch Clearing flags on attachment: 362909 Committed r242065: <https://trac.webkit.org/changeset/242065> All reviewed patches have been landed. Closing bug. |