[webkit-reviews] review granted: [Bug 90511] nrwt: add a MessagePool abstraction that the manager will call to replace the broker : [Attachment 150705] remove unused self.name assignment in manager.__init__()

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 90511: nrwt: add a MessagePool abstraction that the manager will call to
replace the broker
https://bugs.webkit.org/show_bug.cgi?id=90511

Attachment 150705: remove unused self.name assignment in manager.__init__()
https://bugs.webkit.org/attachment.cgi?id=150705&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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 at 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/ ?