WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
90511
nrwt: add a MessagePool abstraction that the manager will call to replace the broker
https://bugs.webkit.org/show_bug.cgi?id=90511
Summary
nrwt: add a MessagePool abstraction that the manager will call to replace the...
Dirk Pranke
Reported
2012-07-03 17:58:29 PDT
nrwt: add a MessagePool abstraction that the manager will call to replace the broker
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Dirk Pranke
Comment 1
2012-07-03 18:03:11 PDT
Created
attachment 150700
[details]
Patch
Dirk Pranke
Comment 2
2012-07-03 18:35:18 PDT
Created
attachment 150705
[details]
remove unused self.name assignment in manager.__init__()
Ojan Vafai
Comment 3
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/ ?
Dirk Pranke
Comment 4
2012-07-11 17:35:44 PDT
Created
attachment 151822
[details]
update w/ review comments
Dirk Pranke
Comment 5
2012-07-11 17:46:07 PDT
Committed
r122394
: <
http://trac.webkit.org/changeset/122394
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug