[webkit-reviews] review denied: [Bug 50557] nrwt multiprocessing - revamp message_broker to actually do messaging : [Attachment 75680] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 50557: nrwt multiprocessing - revamp message_broker to actually do
messaging
https://bugs.webkit.org/show_bug.cgi?id=50557

Attachment 75680: Patch
https://bugs.webkit.org/attachment.cgi?id=75680&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75680&action=review

Mostly some high level first pass comments.  I'm having a hard time seeing the
big picture, but I think it's:
  manager <- manager connection <- broker -> worker connection -> worker
I'm not sure I'm convinced that all these layers are needed.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:109

> +class ClientInterface(object):

Is this the same as BrokerClientInterface?  I don't see anything that inherits
from this class.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:184

> +    try:
> +	   broker_class = broker_types[worker_model]
> +	   return broker_class(port, options, worker_class, worker_topic,
> +			       worker_reply_topic)
> +    except KeyError:

Nit: Can we move the return outside the try block?

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:219

> +    def __init__(self, queue):
> +	   self.queue = queue

Does queue need to be a parameter?  It looks like you always pass in
Queue.Queue().

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:354

> +    def cancel_worker(self, worker_name):

It's a bit odd that start_worker takes a worker_number, but all the other
worker methods take a worker_name.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:372

> +    def add_timer(self, topic_name, callback, delay_secs):
> +	   """Add a timer that will fire later during drain() or loop().

Is this for finding hung threads?  I don't think finding hung threads needs to
be so general purpose.	Why not just name the methods add_hung_thread_timer or
something?

Also, there's a lot of code for timers.  It might be easier to add them in a
separate patch.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:391

> +    def run_message_loop(self, topic_name, reader):
> +	   """Loop processing messages and timers until done.
> +

I think it would be more clear if the broker was less general purpose.	We only
have 2 topics and being able to handle an arbitrary number of topics makes the
code harder to follow.	For example, maybe it would be reasonable to have
run_manager_loop and run_worker_loop?  Actually, I can't tell right now if the
broker class is responsible for running the manager loop.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:392

> +	   "done" means that reader.is_done() returns True or a timer indicates


It would also make "reader" explicit.  Of course, run_manager_loop and
run_worker_loop may both call into a common protected method that does this
abstraction, but the code would be more readable.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:413

> +    def drain(self, topic_name, reader):

Nit: Can we call this run_all_pending?	That's matches MessageLoop in base.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:475

> +	   fn = getattr(reader, 'handle_' + message.name)

fn-> function or callback or message_handler or something

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:480

> +    class _InlineWorkerConnection(object):

I would move this out of the class since it's so big.  It makes it hard to see
which methods are part of _InlineBroker and which methods are part of
_InlineWorkerConnection.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:579

> +    class _ThreadedWorkerConnection(threading.Thread):

I would move this out too.