[
https://issues.apache.org/jira/browse/YARN-4428?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15117530#comment-15117530
]
Jason Lowe commented on YARN-4428:
----------------------------------
Thanks for updating the patch!
I'm not thrilled with the amount of manual string-slinging going on in this
patch, especially since it's so fragile and hard-coded based on what's in other
files (i.e.: RMWebApp#setup). It would be a lot cleaner and more maintainable
if we reused the same logic that's done when parsing URLs for normal dispatch.
Looking at how the webapp Dispatcher works, it uses the router (already in the
RMWebApp) to lookup a destination then uses that destination to parse URL
arguments like app IDs, container IDs, etc. If we had better access at the
webapp router and code in the dispatcher that parsed URL arguments then we
wouldn't have to roll our own here. We could simply reuse that code to figure
out where the URL is going and parse the arguments, then check if we have an
app ID arg or a container ID arg etc. to determine what to do next.
However that's significant work that doesn't need to block this JIRA. Please
file a followup JIRA and reference it in a comment for the new RMWebAppFilter
code to note we should commonize the URL parsing code.
Other comments on the patch:
Note that more than YarnRuntimeException can be thrown when parsing strings as
application IDs. We can also get NumberFormatException, so it's probably safer
to catch Exception around the parse as is done by other web app parsing code.
When IDs fail to parse there should be at least a debug log message stating
what string failed to parse as what ID so it's easier to debug why redirects
aren't happening when people think they should. It should not be common for
IDs to fail to parse given the prefix already seen.
Rather than do conf lookups each and every time we redirect it would be more
efficient to cache this, much like the {{path}} variable is precomputed in the
RMWebAppFilter constructor. We should cache the boolean whether the AHS is
enabled and also the AHS URL prefix (i.e.: http scheme prefix + AHS url without
scheme) which will make the code more efficient and easier to read.
Please investigate the new java warning introduced in the test.
> Redirect RM page to AHS page when AHS turned on and RM page is not avaialable
> -----------------------------------------------------------------------------
>
> Key: YARN-4428
> URL: https://issues.apache.org/jira/browse/YARN-4428
> Project: Hadoop YARN
> Issue Type: Bug
> Reporter: Chang Li
> Assignee: Chang Li
> Attachments: YARN-4428.1.2.patch, YARN-4428.1.patch,
> YARN-4428.2.2.patch, YARN-4428.2.patch, YARN-4428.3.patch, YARN-4428.3.patch,
> YARN-4428.4.patch, YARN-4428.5.patch
>
>
> When AHS is turned on, if we can't view application in RM page, RM page
> should redirect us to AHS page. For example, when you go to
> cluster/app/application_1, if RM no longer remember the application, we will
> simply get "Failed to read the application application_1", but it will be
> good for RM ui to smartly try to redirect to AHS ui
> /applicationhistory/app/application_1 to see if it's there. The redirect
> usage already exist for logs in nodemanager UI.
> Also, when AHS is enabled, WebAppProxyServlet should redirect to AHS page on
> fall back of RM not remembering the app. YARN-3975 tried to do this only when
> original tracking url is not set. But there are many cases, such as when app
> failed at launch, original tracking url will be set to point to RM page, so
> redirect to AHS page won't work.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
