Bug 90511 - nrwt: add a MessagePool abstraction that the manager will call to replace the broker
Summary: nrwt: add a MessagePool abstraction that the manager will call to replace the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 89725
  Show dependency treegraph
 
Reported: 2012-07-03 17:58 PDT by Dirk Pranke
Modified: 2012-07-11 18:41 PDT (History)
5 users (show)

See Also:


Attachments
Patch (20.88 KB, patch)
2012-07-03 18:03 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
remove unused self.name assignment in manager.__init__() (36.87 KB, patch)
2012-07-03 18:35 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review comments (20.33 KB, patch)
2012-07-11 17:35 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-07-03 17:58:29 PDT
nrwt: add a MessagePool abstraction that the manager will call to replace the broker
Comment 1 Dirk Pranke 2012-07-03 18:03:11 PDT
Created attachment 150700 [details]
Patch
Comment 2 Dirk Pranke 2012-07-03 18:35:18 PDT
Created attachment 150705 [details]
remove unused self.name assignment in manager.__init__()
Comment 3 Ojan Vafai 2012-07-11 16:45:05 PDT
Comment on attachment 150705 [details]
remove unused self.name assignment in manager.__init__()

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

Nice cleanup overall. This is heading in the direction of a nice simplification I think.

> Tools/ChangeLog:68
> +2012-07-03  Dirk Pranke  <dpranke@chromium.org>
> +
> +        nrwt: clean up names in worker.py
> +        https://bugs.webkit.org/show_bug.cgi?id=90510

I assume including this was an accident. :)

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:70
> +        self.worker_factory = worker_factory
> +        self.num_workers = num_workers
> +        self.worker_startup_delay_secs = worker_startup_delay_secs
> +        self._worker_states = {}
> +        self.host = host
> +        self.name = 'manager'

Seems like all of these could be private, no?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:118
> +    # FIXME: Inline this function.
> +    def _worker_is_done(self, worker_state):
> +        return worker_state.done

Lol. Adding the FIXME is more work than just doing the inlining, no? :)

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:125
> +        for worker in self._worker_states.values():
> +            if worker.worker_connection.is_alive():
> +                worker.worker_connection.join(5.0)

Nit: This looks like it duplicates 104-110 without the useful logging. Should they use a helper function to share this code? A FIXME would be fine if you agree.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:154
> +    def handle_done(self, source, log_messages=None):
> +        worker_state = self._worker_states[source]
> +        worker_state.done = True
> +        self._log_messages(log_messages)
> +
> +    def handle_started_test(self, *args):
> +        self._caller.handle('started_test', *args)
> +
> +    def handle_finished_test(self, *args):
> +        self._caller.handle('finished_test', *args)
> +
> +    def handle_finished_test_list(self, *args):
> +        self._caller.handle('finished_test_list', *args)
> +
> +    def handle_exception(self, *args):
> +        self._handle_worker_exception(*args)
> +
> +    def _log_messages(self, messages):
> +        for message in messages:
> +            logging.root.handle(message)
> +
> +    @staticmethod
> +    def _handle_worker_exception(source, exception_type, exception_value, stack):
> +        if exception_type == KeyboardInterrupt:
> +            raise exception_type(exception_value)
> +        _log.error("%s raised %s('%s'):" % (
> +                   source, exception_value.__class__.__name__, str(exception_value)))
> +        raise WorkerException(str(exception_value))

These are all unused as best I can tell. Am I looking at the wrong code? I tried code searching for them and I looked at https://bugs.webkit.org/attachment.cgi?id=150711&action=review for uses of them. Actually, it looks like you delete most of these in that followup patch.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:158
> +    """A class for the manager to use to track the current state of the workers."""

Meh to this comment. :) I'd delete it.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:168
> +        self.current_test_name = None
> +        self.next_timeout = None
> +        self.stats = {}
> +        self.stats['name'] = worker_connection.name
> +        self.stats['num_tests'] = 0
> +        self.stats['total_time'] = 0

These all appear unused to me.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:407
>          self._worker_connection.yield_to_broker()

FIXME s/yield_to_broker/yield_to_caller/ ?
Comment 4 Dirk Pranke 2012-07-11 17:35:44 PDT
Created attachment 151822 [details]
update w/ review comments
Comment 5 Dirk Pranke 2012-07-11 17:46:07 PDT
Committed r122394: <http://trac.webkit.org/changeset/122394>