Summary: | [ews-app] Update primary keys for handling multiple Buildbot instances | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
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, ews-watchlist, 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=194863 | ||||||||||
Attachments: |
|
Description
Aakash Jain
2019-02-27 14:04:53 PST
Created attachment 363130 [details]
Proposed patch
Tested on a local instance. This will apply to ToT after https://bugs.webkit.org/show_bug.cgi?id=195047 Comment on attachment 363130 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=363130&action=review > Tools/BuildSlaveSupport/ews-app/ews/models/build.py:38 > + uid = models.TextField(primary_key=True) So, build_id's are unique per instance, but Build uid's combine build_id with builder_id, right? Why do we still have builder_id in this Model, then? > So, build_id's are unique per instance Yes > but Build uid's combine build_id with builder_id, right? I assume you meant combine build_id with buildbot_instance_id. If so, yes. > Why do we still have builder_id in this Model, then? To generate buildbot url's. Created attachment 363248 [details]
Proposed patch
Included Buildbot(ews-build) changes to send hostname along-with the events data.
Attachment 363248 [details] did not pass style-queue:
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:45: [BuildbotInstance.get_instance_id] Class 'BuildbotInstance' has no 'objects' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:46: [BuildbotInstance.get_instance_id] Class 'BuildbotInstance' has no 'DoesNotExist' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:48: [BuildbotInstance.get_instance_id] Instance of 'BuildbotInstance' has no 'save' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/build.py:65: [Build.save_build] Instance of 'Build' has no 'save' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/build.py:94: [Build.get_existing_build] Class 'Build' has no 'objects' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/build.py:95: [Build.get_existing_build] Class 'Build' has no 'DoesNotExist' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/step.py:63: [Step.save_step] Instance of 'Step' has no 'save' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/step.py:83: [Step.get_existing_step] Class 'Step' has no 'objects' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/step.py:84: [Step.get_existing_step] Class 'Step' has no 'DoesNotExist' member [pylint/E1101] [5]
Total errors found: 9 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Aakash Jain from comment #4) > > So, build_id's are unique per instance > Yes > > but Build uid's combine build_id with builder_id, right? > I assume you meant combine build_id with buildbot_instance_id. If so, yes. Actually, that is the source of my confusion, I was thinking builder_id and buildbot_instance_id were the same thing, they aren't. > > > Why do we still have builder_id in this Model, then? > To generate buildbot url's. Comment on attachment 363248 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=363248&action=review Mostly looks good; slightly concerned about combining instance_id and build_or_step_id into another field. > Tools/BuildSlaveSupport/ews-app/ews/models/build.py:94 > + return Build.objects.get(uid=uid) Is it necessary to use a try/except block here? I would expect `Build.objects.get` to return None if the build didn't exist (without throwing any exception). > Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:55 > + return '{}_{}'.format(instance_id, build_or_step_id) What benefit do we get by combining instance_id with build_or_step_id? I'm concerned we'll have performance issues caused by trying to look up information by this uid given this makes it not possible to index on the instance_id directly. Also, what happens if build_id and step_id collide for the same instance? Wouldn't that result in corruption or data loss? > Tools/BuildSlaveSupport/ews-app/ews/models/step.py:84 > + except Step.DoesNotExist: Ditto to Build comment. > Tools/BuildSlaveSupport/ews-build/events.py:76 > + def __init__(self, hostname, type_prefix='', name='Events'): It's a bit unclear to me if hostname is the worker, or the instance hostname. Could you rename this to instance_hostname or worker_hostname for clarity? Comment on attachment 363248 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=363248&action=review >> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:94 >> + return Build.objects.get(uid=uid) > > Is it necessary to use a try/except block here? I would expect `Build.objects.get` to return None if the build didn't exist (without throwing any exception). Build.objects.get does throw an exception if the object doesn't exist. https://docs.djangoproject.com/en/2.1/topics/db/queries/#retrieving-a-single-object-with-get "If there are no results that match the query, get() will raise a DoesNotExist exception." >> Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:55 >> + return '{}_{}'.format(instance_id, build_or_step_id) > > What benefit do we get by combining instance_id with build_or_step_id? I'm concerned we'll have performance issues caused by trying to look up information by this uid given this makes it not possible to index on the instance_id directly. > > Also, what happens if build_id and step_id collide for the same instance? Wouldn't that result in corruption or data loss? Django doesn't seems to support composite/multiple-column primary keys: https://code.djangoproject.com/ticket/373 If we get the same uid (same buildbot_instance_id and build_id) for a build, we further check if the patch_id, builder_id, number etc. are same as existing data or not. If not, it would fail the check here and would simply refuse the new data: https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-app/ews/models/build.py#L70 Further, buildbot should indicate that failure in the UI and mark that build at failure/retry (that buildbot portion is not yet implemented). >> Tools/BuildSlaveSupport/ews-build/events.py:76 >> + def __init__(self, hostname, type_prefix='', name='Events'): > > It's a bit unclear to me if hostname is the worker, or the instance hostname. Could you rename this to instance_hostname or worker_hostname for clarity? This is the Buildbot master code. I can rename it to instance_hostname, however 'instance_hostname' might seem unusual, given that any buildbot instance doesn't know about any other instance. Maybe 'buildbot_hostname' is better. (In reply to Aakash Jain from comment #9) > Comment on attachment 363248 [details] > Proposed patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363248&action=review > > >> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:94 > >> + return Build.objects.get(uid=uid) > > > > Is it necessary to use a try/except block here? I would expect `Build.objects.get` to return None if the build didn't exist (without throwing any exception). > > Build.objects.get does throw an exception if the object doesn't exist. > > https://docs.djangoproject.com/en/2.1/topics/db/queries/#retrieving-a-single- > object-with-get > "If there are no results that match the query, get() will raise a > DoesNotExist exception." > Ah, okay. Thank you for explaining. I don't like that paradigm at all, but guess we're tied to it since we're using Django. :) > >> Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:55 > >> + return '{}_{}'.format(instance_id, build_or_step_id) > > > > What benefit do we get by combining instance_id with build_or_step_id? I'm concerned we'll have performance issues caused by trying to look up information by this uid given this makes it not possible to index on the instance_id directly. > > > > Also, what happens if build_id and step_id collide for the same instance? Wouldn't that result in corruption or data loss? > > Django doesn't seems to support composite/multiple-column primary keys: > https://code.djangoproject.com/ticket/373 > > If we get the same uid (same buildbot_instance_id and build_id) for a build, > we further check if the patch_id, builder_id, number etc. are same as > existing data or not. If not, it would fail the check here and would simply > refuse the new data: > https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-app/ > ews/models/build.py#L70 Sorry, that wasn't quite my question. What if the second id (sourced from 'build_id' or 'step_id') collides since they're coming from two sources? I also think it would be better to use a monotonically increasing ID with two separate columns for instanceid and build_or_step_id (pending the other issue is resolved) instead of this combination of two keys, so you can still index on instanceid. > > Further, buildbot should indicate that failure in the UI and mark that build > at failure/retry (that buildbot portion is not yet implemented). > > >> Tools/BuildSlaveSupport/ews-build/events.py:76 > >> + def __init__(self, hostname, type_prefix='', name='Events'): > > > > It's a bit unclear to me if hostname is the worker, or the instance hostname. Could you rename this to instance_hostname or worker_hostname for clarity? > > This is the Buildbot master code. I can rename it to instance_hostname, > however 'instance_hostname' might seem unusual, given that any buildbot > instance doesn't know about any other instance. Maybe 'buildbot_hostname' is > better. Oh, this is Buildbot EWS, not the OpenSource EWS Django-based service? If so, "master_hostname" would be a bit more clear to me. Spoke with Aakash on the phone. Patch looks good; unofficial r+. Created attachment 363376 [details]
Proposed patch
Renamed hostname to master_hostname.
Attachment 363376 [details] did not pass style-queue:
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:45: [BuildbotInstance.get_instance_id] Class 'BuildbotInstance' has no 'objects' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:46: [BuildbotInstance.get_instance_id] Class 'BuildbotInstance' has no 'DoesNotExist' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/buildbotinstance.py:48: [BuildbotInstance.get_instance_id] Instance of 'BuildbotInstance' has no 'save' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/build.py:65: [Build.save_build] Instance of 'Build' has no 'save' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/build.py:94: [Build.get_existing_build] Class 'Build' has no 'objects' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/build.py:95: [Build.get_existing_build] Class 'Build' has no 'DoesNotExist' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/step.py:63: [Step.save_step] Instance of 'Step' has no 'save' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/step.py:83: [Step.get_existing_step] Class 'Step' has no 'objects' member [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/step.py:84: [Step.get_existing_step] Class 'Step' has no 'DoesNotExist' member [pylint/E1101] [5]
Total errors found: 9 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 363376 [details] Proposed patch Clearing flags on attachment: 363376 Committed r242291: <https://trac.webkit.org/changeset/242291> All reviewed patches have been landed. Closing bug. |