Bug 46141 – Add a feeder queue that polls bugs.webkit.org for the commit-cluster

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Add a feeder queue that polls bugs.webkit.org for the commit-cluster

[Adam Barth]

Created attachment 68161 [details]
wip

[Adam Barth]

Created attachment 68165 [details]
work in progress

[Adam Barth]

Created attachment 68171 [details]
Patch

[WebKit Review Bot]

Attachment 68171 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:38:  deprecated form of raising exception  [pep8/W602] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 68171 [details]
Patch

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

In general looks great!  Please address the nits above.

> WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:35
> +        self.tool = tool

Why not _tool? :)

> WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:55
> +        self.update_work_items([patch.id() for patch in patches])

I might have put this out into a patch_ids local (I know it wasn't before).  Doesn't really matter.

> WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:60
> +        all_patches = sum([self.tool.bugs.fetch_bug(bug_id).commit_queued_patches(include_invalid=True) for bug_id in bug_ids], [])

I might have split this out into a helper function to make this line shorter were I to write this again.  (Again, I know this is my fault, just noticing as I re-read the code):

[self._cq_patches_for_bug(bug_id) for bug_id in bug_ids]?  donno.  Again, doesn't really matter.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:149
> +class FeederQueue(AbstractQueue):

I think you should add a comment above which explains why this makes sense as an AbstractQueue.  It's not immediately obvious, given that it's such a strange queue.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:160
> +        self.feeders = [
> +            CommitQueueFeeder(self.tool),
> +        ]

This is nice.  I wonder if it should be a list of classes instead of instances.  I seem to remember having the same debate with myself for Commands.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:163
> +        return "synthetic-work-item"

I would have added a comment here.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:166
> +        return True

Seems this should just be the default.  Silly to make subclasses implement this.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:169
> +        self._update()

_update_checkout() maybe?

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:173
> +        for feeder in self.feeders:
> +            feeder.feed()
> +        time.sleep(self._sleep_duration)
> +        return True

So gorgeous. :)

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:176
> +    def work_item_log_path(self, work_item):
> +        return os.path.join(self._log_directory(), "feeder.log")

This is a strange case.  We'll end up loggin the same thing twice.  Maybe this should instead understand returning None and then not opening a work log?

> WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py:113
> +        queue = FeederQueue()
> +        queue._sleep_duration = 0

This should probably be a FeederQueue subclass since many tests are going to want this same behavior, no?

> WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py:197
> +            "next_work_item": "",

Are these empty ones really required?  I can't remember.

> WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py:104
> +        if isinstance(queue, StepSequenceErrorHandler):

Seems we should add a comment about future refactoring here.

[Adam Barth]

Committed r67913: <http://trac.webkit.org/changeset/67913>