nrwt: add a MessagePool abstraction that the manager will call to replace the broker
Created attachment 150700 [details] Patch
Created attachment 150705 [details] remove unused self.name assignment in manager.__init__()
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/ ?
Created attachment 151822 [details] update w/ review comments
Committed r122394: <http://trac.webkit.org/changeset/122394>