[ 
https://issues.apache.org/jira/browse/YARN-2030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14004398#comment-14004398
 ] 

Jian He commented on YARN-2030:
-------------------------------

Hi Binglin, thanks for cleaning it up ! 
Just quickly looked at the patch. some suggestion. I think ApplicationStateData 
can be changed to an abstract class just like many other records and add the 
newInstance() method. So things like ApplicationStateDataPBImpl don't need the 
newApplicationStateData method. Accordingly storeApplicationStateInternal can 
take in  ApplicationStateData instead of ApplicationStateDataPBImpl as the 
argument to avoid the type cast.

> Use StateMachine to simplify handleStoreEvent() in RMStateStore
> ---------------------------------------------------------------
>
>                 Key: YARN-2030
>                 URL: https://issues.apache.org/jira/browse/YARN-2030
>             Project: Hadoop YARN
>          Issue Type: Improvement
>            Reporter: Junping Du
>            Assignee: Binglin Chang
>         Attachments: YARN-2030.v1.patch, YARN-2030.v2.patch
>
>
> Now the logic to handle different store events in handleStoreEvent() is as 
> following:
> {code}
> if (event.getType().equals(RMStateStoreEventType.STORE_APP)
>         || event.getType().equals(RMStateStoreEventType.UPDATE_APP)) {
>       ...
>       if (event.getType().equals(RMStateStoreEventType.STORE_APP)) {
>         ...
>       } else {
>         ...
>       }
>       ...
>       try {
>         if (event.getType().equals(RMStateStoreEventType.STORE_APP)) {
>           ...
>         } else {
>           ...
>         }
>       } 
>       ...
>     } else if (event.getType().equals(RMStateStoreEventType.STORE_APP_ATTEMPT)
>         || event.getType().equals(RMStateStoreEventType.UPDATE_APP_ATTEMPT)) {
>       ...
>       if (event.getType().equals(RMStateStoreEventType.STORE_APP_ATTEMPT)) {
>         ...
>       } else {
>         ...
>       }
>         ...
>         if (event.getType().equals(RMStateStoreEventType.STORE_APP_ATTEMPT)) {
>           ...
>         } else {
>           ...
>         }
>       }
>       ...
>     } else if (event.getType().equals(RMStateStoreEventType.REMOVE_APP)) {
>     ...
>     } else {
>       ...
>     }
> }
> {code}
> This is not only confuse people but also led to mistake easily. We may 
> leverage state machine to simply this even no state transitions.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Reply via email to