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

Robert Kanter commented on YARN-4946:
-------------------------------------

{quote}Please also keep in mind that checkAppNumCompletedLimit is only invoked 
when an app becomes completed (APP_COMPLETED event dispatched from 
FinalTransition), so it can happen that we store more applications in the 
state-store and memory until another app finishes, but I can't think of any 
better solution currently.{quote}
I think that's fine.  The pressure to remove older completed apps comes from 
newer apps being completed, so if there are no new apps completed, we don't 
need to worry about this.  And when a newer app completes, this will trigger.

The 003 patch looks good overall.  Here's some more comments:
# Missing a newline at the end of {{RMAppImplTest}}.
# {{RMAppImplTest}} should be named {{TestRMAppImpl}} (we typically name tests 
{{TestFoo}} rather than {{FooTest}}).
# In {{RMAppImplTest}}, it would be best if we could avoid so much reflection 
to get access to internal state.  Instead, perhaps we can either get the App 
into that state by sending events (it looks like {{TestRMAppTransitions}} does 
this) or a {{\@VisibleForTesting}} method.  
#- Similarly, with {{completedAppsField}} in 
{{TestRMAppManager$TestRMAppManager}}.  It would be better to add a getter 
that's {{\@VisibleForTesting}} and package private.  Or given that 
{{TestRMAppManager$TestRMAppManager}} is a subclass of {{RMAppManager}}, you 
could make it protected.
# I'm not sure we need so many tests in {{RMAppImplTest}}.  The updated code in 
{{RMAppImpl#FinalTransition}} doesn't really do much with log aggregation now, 
and does pretty much the same thing whether it's enabled or not.  So having 
tests that check the behavior with various states of log aggregation seems 
unnecessary.  
#- In fact, the changes in {{RMAppImpl#FinalTransition}} are all just 
re-organizing the code - the logic is still the same, right?  And it's not 
really even related to this JIRA.  Perhaps these changes should be moved to a 
new JIRA that's aim is to make the code more readable?
# {{RMAppManager}} changes and tests look good.  
# The line declaring 
{{TestAppManager#createRMAppsMapMixedLogAggregationStatus}} is too long.


> RM should not consider an application as COMPLETED when log aggregation is 
> not in a terminal state
> --------------------------------------------------------------------------------------------------
>
>                 Key: YARN-4946
>                 URL: https://issues.apache.org/jira/browse/YARN-4946
>             Project: Hadoop YARN
>          Issue Type: Improvement
>          Components: log-aggregation
>    Affects Versions: 2.8.0
>            Reporter: Robert Kanter
>            Assignee: Szilard Nemeth
>            Priority: Major
>         Attachments: YARN-4946.001.patch, YARN-4946.002.patch, 
> YARN-4946.003.patch
>
>
> MAPREDUCE-6415 added a tool that combines the aggregated log files for each 
> Yarn App into a HAR file.  When run, it seeds the list by looking at the 
> aggregated logs directory, and then filters out ineligible apps.  One of the 
> criteria involves checking with the RM that an Application's log aggregation 
> status is not still running and has not failed.  When the RM "forgets" about 
> an older completed Application (e.g. RM failover, enough time has passed, 
> etc), the tool won't find the Application in the RM and will just assume that 
> its log aggregation succeeded, even if it actually failed or is still running.
> We can solve this problem by doing the following:
> The RM should not consider an app to be fully completed (and thus removed 
> from its history) until the aggregation status has reached a terminal state 
> (e.g. SUCCEEDED, FAILED, TIME_OUT).



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org

Reply via email to