Jason Lowe commented on YARN-3975:

Thanks for updating the patch, Mit!

Having two separate booleans for RM vs. AHS seems a bit odd and leads to weird 
situations like the possibiltiy they're both true.  Normally this is handled 
via an enum which would make the code a bit cleaner and eliminate the potential 
for weird states.

I think it would be a bit clearer if ApplicationReportBundle were named 
something like FetchedAppReport or something similar.

To reduce the amount of unnecessary boilerplate associated with the new class, 
I think the constructor should take the app report and the origination enum and 
there should only be getters.  We should be able to treat a fetched application 
report like an immutable object, and I don't see a need to have setters on it.

Nit: a number of comments that were added in the patch are exactly parroting 
the code and aren't adding value, e.g.:
      //set this flag to true so that WebAppProxyServlet can understand that the
      //appReport was fetched from the RM
      isReportFromRM = true;
    //Create a bundle of appReport and the source from where it was fetched
    //so the WebAppProxyServlet has a way to know the appReport source
    ApplicationReportBundle appReportBundle  = new ApplicationReportBundle();

Rather than tack on another set of tests to an already involved test method, it 
would be cleaner to have a separate test case for the new functionality.

> WebAppProxyServlet should not redirect to RM page if AHS is enabled
> -------------------------------------------------------------------
>                 Key: YARN-3975
>                 URL: https://issues.apache.org/jira/browse/YARN-3975
>             Project: Hadoop YARN
>          Issue Type: Bug
>    Affects Versions: 2.7.1
>            Reporter: Mit Desai
>            Assignee: Mit Desai
>         Attachments: YARN-3975.2.b2.patch, YARN-3975.3.patch, 
> YARN-3975.4.patch, YARN-3975.5.patch, YARN-3975.6.patch
> WebAppProxyServlet should be updated to handle the case when the appreport 
> doesn't have a tracking URL and the Application History Server is eanbled.
> As we would have already tried the RM and got the 
> ApplicationNotFoundException we should not direct the user to the RM app page.

This message was sent by Atlassian JIRA

Reply via email to