Bug 90511 – nrwt: add a MessagePool abstraction that the manager will call to replace the broker

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: add a MessagePool abstraction that the manager will call to replace the broker

[Dirk Pranke]

Created attachment 150700 [details]
Patch

[Dirk Pranke]

Created attachment 150705 [details]
remove unused self.name assignment in manager.__init__()

[Ojan Vafai]

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
> [phone]  Dirk Pranke  <[e-mail]>
> +
> +        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/ ?

[Dirk Pranke]

Created attachment 151822 [details]
update w/ review comments

[Dirk Pranke]

Committed r122394: <http://trac.webkit.org/changeset/122394>