[
https://issues.apache.org/jira/browse/HDDS-50?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16476503#comment-16476503
]
Elek, Marton commented on HDDS-50:
----------------------------------
Thank you very much the patch [~msingh]. First of all I would like to state
that I am not against this approach. But I would like to make it clean that
this patch not only adds a priority queue but changes the actor-like
architecture to a simple thread pool architecture (which is very similar to the
existing Hadoop approach and slightly different to the original scm proposal).
What I would like to do is just a clarification what did I think about the
original approach (which was based on my limited understanding of reactive
patterns and experiences with actor based systems like Akka).
1. Priority is a good example. Imagine that you have multiple independent
EventHandlers, each with a separated thread/executor. We can name the
EventHandlers as actors and the dedicated thread pools as mailboxes, but it
doesn’t matter. The important thing is that each EventHandler has a dedicated
mailboxes and EventHandlers could communicate with each other only with sending
messages to the other’s mailboxes.
Most probably you will have a facade EventHandler (with dedicated thread pool)
which routes the incomming messages to the executor/mailbox of the dedicated
event handlers.
Here the priority is not so important. The facade EventHandler will be very
fast and it will process all the message immediately as the processing means
only to move the message to the exexutor/mailbox of a dedicated event handler.
There could be separated EventHandler for MISSING_NODE event, or
MISSING_REPLICA event, but if each has an own mailbox, we may not need a
priority inside the dedicated mailboxes. (You can set priorities with
dedicating more thread to an actor.)
So priority is a nice to have not a must have.
2. In an actor based model each EventHandler (actor) has a dedicated mailbox,
and the they try to process the incomming messages as fast as possible (ideally
all the IO and external calls from an actor are async).
But using Future doesn’t guarantee full async calls. Fix me if I am wrong, but
I think Future.get will block the current thread while the Future will be
finished. If an actor/EventHandler (which has a single thread executor mailbox)
would call Future.get, all the processing will be stopped until Future.get will
continue.
So using Future could be dangerous.
Sometimes it could be usefull as by default in an actor system there is no
request/response, just one-way messages. Creating request/response we need to
send a response message to the original actor with some correlation information
OR using Future, but in that case Future should contain some response message
not just Void (and still dangerous).
This patch suggests to return with Future<Void>. I can see the danger but no
exact benefit.
Summary: This patch not only add priorities but also changes my original
proposal and switch back to a single executor based model instead of a full
async approach. I can accept this approach, both could work (but personally I
think the full async method could be more effective, even if the programming
model may be more complex).
Some other comments which are independent from the execution model:
a) Until now the event-type/event-name/topic(=Event) was separated from the
payload(=PAYLOAD). The Event was the same for all the messages but each
messages has a different payload. It was based on IRL discussion to support a
use-case where different type of events have the same payload.
This patch suggests to add priority to the Event itself. So we need a per
message event object. (One EventHandler could receive multiple payloads with
different priorities and priority is propagated inside the Event).
I am not totally against this approach, but in this case payload also could be
added to the Event itself. Why do we need separated payload?
b) NICE_XXX enum members in the EventPriority is a little strange for me. Why
don't we use simple int? It seems that we can't name the different priorities.
c) It would be great to find a way to unit-test the priorities. Currently only
the compare method of the EventTask is tested, but I can’t see real-word
fire-event/receive-event unit tests where the result depends from the
prioritiezed message order.
> EventQueue: Add a priority based execution model for events in eventqueue.
> --------------------------------------------------------------------------
>
> Key: HDDS-50
> URL: https://issues.apache.org/jira/browse/HDDS-50
> Project: Hadoop Distributed Data Store
> Issue Type: Bug
> Components: SCM
> Affects Versions: 0.2.1
> Reporter: Mukul Kumar Singh
> Assignee: Mukul Kumar Singh
> Priority: Major
> Fix For: 0.2.1
>
> Attachments: HDDS-50.001.patch, HDDS-50.002.patch, HDDS-50.003.patch,
> HDDS-50.004.patch, HDDS-50.005.patch
>
>
> Currently all the events in SCM are executed with the same priority. This
> jira will add a priority based execution model where the "niceness" value of
> an event will determine the priority of the execution of the event.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
