Sangjin Lee commented on YARN-3367:

Thanks for the patch [~Naganarasimha]! I took a fairly quick look at the latest 

One high level comment: as for the async dispatcher, I would very much advocate 
using the JDK's ExecutorService (single-threaded executor in this case) over 
using a raw thread and its own blocking queue management. It will definitely 
reduce the amount of code (and room for errors), and we can focus on the actual 
unit of work that needs to be done. Can you please consider using an 
ExecutorService over the thread + queue? If there is a compelling reason that 
an ExecutorService cannot work, I'd be curious to learn.

On TimelineEntities.java,
- hashCode(): why not simply return entities.hashCode()? entities is never null
- equals(): again, note that entities is never null; that will simplify the 
implementation here

> Replace starting a separate thread for post entity with event loop in 
> TimelineClient
> ------------------------------------------------------------------------------------
>                 Key: YARN-3367
>                 URL: https://issues.apache.org/jira/browse/YARN-3367
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>    Affects Versions: YARN-2928
>            Reporter: Junping Du
>            Assignee: Naganarasimha G R
>              Labels: yarn-2928-1st-milestone
>         Attachments: YARN-3367-feature-YARN-2928.003.patch, 
> YARN-3367-feature-YARN-2928.v1.002.patch, 
> YARN-3367-feature-YARN-2928.v1.004.patch, YARN-3367.YARN-2928.001.patch
> Since YARN-3039, we add loop in TimelineClient to wait for 
> collectorServiceAddress ready before posting any entity. In consumer of  
> TimelineClient (like AM), we are starting a new thread for each call to get 
> rid of potential deadlock in main thread. This way has at least 3 major 
> defects:
> 1. The consumer need some additional code to wrap a thread before calling 
> putEntities() in TimelineClient.
> 2. It cost many thread resources which is unnecessary.
> 3. The sequence of events could be out of order because each posting 
> operation thread get out of waiting loop randomly.
> We should have something like event loop in TimelineClient side, 
> putEntities() only put related entities into a queue of entities and a 
> separated thread handle to deliver entities in queue to collector via REST 
> call.

This message was sent by Atlassian JIRA

Reply via email to