Bug 53158

Created attachment 80164 [details] Patch

Created attachment 81021 [details] add more comments describing the design

Comment on attachment 81021 [details] add more comments describing the design View in context: https://bugs.webkit.org/attachment.cgi?id=81021&action=review r- because I still don't have a good sense of how these classes fit together. > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:31 > +"""Module for handling messaging for run-webkit-tests. > + > +This modules implements a simple message broker abstraction that will be This docstring seems overly long and it seems like it could be done simply as a ASCII art diagram with a couple sentences of text. Having read the docstring a few times, I still don't have a good idea of how these classes interact. > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:76 > +class ClientInterface(object): > + """Abstract base class / interface that all message broker clients must Nit: I would maybe just name this BrokerClient. That seems to be more consistent with the naming WebKit/WebCore. > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:116 > + Args: > + options: a runtime option class from optparse Nit: Can you document queue_maker? > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:124 > + def add_topic(self, topic_name): > + if topic_name not in self._topics: > + self._topics[topic_name] = self._queue_maker() Do we change topics on the fly or is the list of topics something that can be passed into the ctor? > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:127 > + def _find_topic(self, topic_name): > + return self._topics[topic_name] Nit: Maybe _get_topic_queue or even _get_message_queue_for_topic?

(In reply to comment #3) > (From update of attachment 81021 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81021&action=review > > r- because I still don't have a good sense of how these classes fit together. > > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:31 > > +"""Module for handling messaging for run-webkit-tests. > > + > > +This modules implements a simple message broker abstraction that will be > > This docstring seems overly long and it seems like it could be done simply as a ASCII art diagram with a couple sentences of text. Having read the docstring a few times, I still don't have a good idea of how these classes interact. I'm not sure that ASCII art would really help. I did a couple of drawings in Google Docs that I sent you the links to, and am attaching PDFs of them here ... Feel free to try and condense them into ASCII :) > > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:76 > > +class ClientInterface(object): > > + """Abstract base class / interface that all message broker clients must > > Nit: I would maybe just name this BrokerClient. That seems to be more consistent with the naming WebKit/WebCore. > Okay, I don't think either ClientInterface or BrokerClient are great names, but I'm happy to change. I'll wait to dicuss the patch with you further before updating the code. > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:116 > > + Args: > > + options: a runtime option class from optparse > > Nit: Can you document queue_maker? > Whoops. Will do. > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:124 > > + def add_topic(self, topic_name): > > + if topic_name not in self._topics: > > + self._topics[topic_name] = self._queue_maker() > > Do we change topics on the fly or is the list of topics something that can be passed into the ctor? > The way the code is written now, we only create two topics, immediately after creating the broker, so they could be passed into the constructor if so desired. > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:127 > > + def _find_topic(self, topic_name): > > + return self._topics[topic_name] > > Nit: Maybe _get_topic_queue or even _get_message_queue_for_topic? Sure, I can make that change.

Created attachment 81167 [details] basic object diagram of the main classes in NRWT pre-multiprocessing rewrite

Created attachment 81168 [details] object diagram post- multiprocessing rewrite

Note that it may help things to combine message_broker2.py and manager_worker_broker.py into a single file, since they're kinda related, and only manager_worker_broker calls into message_broker2. I initially kept them separate under the theory that that might make things easier to understand (and because WebKit seems to like the one-big-class-per-file model)

Created attachment 81302 [details] update with review feedback from tony

Created attachment 81308 [details] fix typos

Created attachment 81309 [details] object diagram post- multiprocessing rewrite updated diagram to be correct about who talks to who between clients, brokerconnections, and brokers.

Comment on attachment 81308 [details] fix typos View in context: https://bugs.webkit.org/attachment.cgi?id=81308&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:50 > + TestRunner ------> BrokerConnection So TestRunner implements BrokerClient? It might be worthwhile to make that explicit. > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:82 > + def __init__(self, broker_connection, worker_number, options, **kwargs): There's a bunch of worker references in the BrokerClient class that seem out of place (i.e. at the wrong abstraction layer). If some (most?) BrokerClient implementations are going to be workers, perhaps it makes sense to have a WorkerBrokerClient abstract class (in a different file) that subclasses BrokerClient has the cancel() and run() abstract methods. > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:108 > + """Brokers provide the basic model of named queues. Clients can post a There's some inconsistency in naming (topics vs. named queues), both in comments and code. I think consistency would help (I have a slight preference for "topic", since that's more common in publish-subscribe systems). > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:129 > + def post_message(self, client, topic_name, message_name, *optargs): Call optargs message_args for consistency with BrokerConnection. > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:140 > + self._run_loop(topic_name, client, True, delay_secs) Use block=True so that it's easier to read the call. > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:201 > + def client(self): Is there a significant benefit from forcing users of this package to subclass BrokerConnection to provide a client, instead of just making it a constructor parameter?

(In reply to comment #10) > Created an attachment (id=81309) [details] > object diagram post- multiprocessing rewrite > > updated diagram to be correct about who talks to who between clients, brokerconnections, and brokers. Seems like the diagram needs to be updated again (e.g. I think ClientInterface is now BrokerClient).

Comment on attachment 81308 [details] fix typos View in context: https://bugs.webkit.org/attachment.cgi?id=81308&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2_unittest.py:67 > + def test_clientinterface_is_abstract(self): Should be renamed to test_brokerclient_is_abstract (also, I'm not sure if this is a worthwhile test, are we that concerned about 100% coverage?)

(In reply to comment #11) > (From update of attachment 81308 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81308&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:50 > > + TestRunner ------> BrokerConnection > > So TestRunner implements BrokerClient? It might be worthwhile to make that explicit. > TestRunner doesn't, actually, but TestRunner2 will eventually. I've updated the docstring to use "BrokerClient" for now. > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:82 > > + def __init__(self, broker_connection, worker_number, options, **kwargs): > > There's a bunch of worker references in the BrokerClient class that seem out of place (i.e. at the wrong abstraction layer). If some (most?) BrokerClient implementations are going to be workers, perhaps it makes sense to have a WorkerBrokerClient abstract class (in a different file) that subclasses BrokerClient has the cancel() and run() abstract methods. Yeah. Mostly the comments were wrong, left over from a previous incarnation where BrokerClient and Workers weren't really different. I've cleaned things up. > > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:108 > > + """Brokers provide the basic model of named queues. Clients can post a > > There's some inconsistency in naming (topics vs. named queues), both in comments and code. I think consistency would help (I have a slight preference for "topic", since that's more common in publish-subscribe systems). Agreed. I've used topic. > > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:129 > > + def post_message(self, client, topic_name, message_name, *optargs): > > Call optargs message_args for consistency with BrokerConnection. > Good suggestion. Done. > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:140 > > + self._run_loop(topic_name, client, True, delay_secs) > > Use block=True so that it's easier to read the call. > Done. > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:201 > > + def client(self): > > Is there a significant benefit from forcing users of this package to subclass BrokerConnection to provide a client, instead of just making it a constructor parameter? Sorry, client was supposed to be passed into the constructor as you say, and callers do not subclass BrokerConnection normally (although manager_worker_broker does). (In reply to comment #13) > > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2_unittest.py:67 > > + def test_clientinterface_is_abstract(self): > > Should be renamed to test_brokerclient_is_abstract (also, I'm not sure if this is a worthwhile test, are we that concerned about 100% coverage?) Renamed. I personally try to get 100% coverage, since it makes it much easier to spot regressions in coverage.

Created attachment 81582 [details] update w/ review feedback from mihaip

Comment on attachment 81582 [details] update w/ review feedback from mihaip View in context: https://bugs.webkit.org/attachment.cgi?id=81582&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:53 > + BrokerClient ------> BrokerConnection > + < | > + \ v > + Broker This might be clearer: BrokerClient ------> BrokerConnection ^ | | v \---------------- Broker > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:59 > + Nit: Remove blank line

(In reply to comment #16) > (From update of attachment 81582 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81582&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:53 > > + BrokerClient ------> BrokerConnection > > + < | > > + \ v > > + Broker > > This might be clearer: > BrokerClient ------> BrokerConnection > ^ | > | v > \---------------- Broker > Good idea. > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:59 > > + > > Nit: Remove blank line Will do.

Committed r77989: <http://trac.webkit.org/changeset/77989>

Comment on attachment 81582 [details] update w/ review feedback from mihaip View in context: https://bugs.webkit.org/attachment.cgi?id=81582&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:136 > + queue = self._find_topic(topic_name) You might have fixed this in a later patch, but _find_topic is not _get_queue_for_topic

(In reply to comment #19) > (From update of attachment 81582 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81582&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:136 > > + queue = self._find_topic(topic_name) > > You might have fixed this in a later patch, but _find_topic is not _get_queue_for_topic Yeah, that one slipped through :( It'll get fixed in the next patch I upload.