[
https://issues.apache.org/jira/browse/TEZ-2018?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14303803#comment-14303803
]
Hitesh Shah commented on TEZ-2018:
----------------------------------
Comments:
- why is the jetty version downgraded?
- why does a user need to configure both "TEZ_HISTORY_URL_BASE" and
"TEZ_AM_TEZ_UI_HISTORY_URL_TEMPLATE" ? Is TEZ_AM_TEZ_UI_HISTORY_URL_TEMPLATE
meant for internal use only?
- Documentation needs to be more clear as it references internal class
fields instead of config properties
- why is this TEZ_AM_WEBSERVICE_ENABLE needed?
- Is hadoop-yarn-server-web-proxy a public or a private package? Does yarn
guarantee compatibility?
{code}
String historyUrl;
524 if (webUI != null) {
525 historyUrl = webUI.getHistoryUrl();
526 } else {
527 historyUrl = "";
528 }
{code}
- could be re-written to "()? x : y; " similar to trackingUrl handling
earlier in the code.
{code}
origin = url.getProtocol() + "://" + url.getAuthority();
{code}
- Will there be issues when handling https? If yes, please file a follow-up
jira to support https
- Please wrap all "LOG.debug" within a if (LOG.isDebugEnabled())
{code}
sendErrorResponse(HttpServletResponse.SC_UNAUTHORIZED, "Access denied",
null);
{code}
- Might be good to have the message contain the user id.
{code}
pw.write("<p>The TEZ UI is hosted at <a href=\"" + historyUrl + "\">"
314 + historyUrl + "</href>");
{code}
- does this message need to be modified to mention that the page will
automatically redirect or to click the link if it does not redirect?
{code}
if (historyUrl == null || historyUrl.isEmpty()) {
...
// set the tracking url only if history url is set, as the index page will
redirect to
// history url
{code}
- Better handling for this case is needed. The trackingUrl for the AM should
still be set but the html which is rendered should inform the user that a
history url was not configured and also tell the user what config property
should be set.
{code}
// Explicitly disabling SSL for map reduce task as we can't allow AM users
{code}
- there is a map reduce related comment. Copy-paste error?
{code}
boolean atsEnabled =
config.getBoolean(YarnConfiguration.TIMELINE_SERVICE_ENABLED, false);
{code}
- why are we relying on a yarn config? If tez is configured to use the
timeline logging service, that should suffice as a check.
{code}
historyUrl = historyUrlTemplate
157 .replaceAll(APPLICATION_ID_PLACEHOLDER,
this.context.getApplicationID().toString())
158 .replaceAll(HISTORY_URL_BASE, historyUrlBase);
{code}
- Can HISTORY_URL_BASE itself be a template?
- Do we really need both vertexProgresses() and vertexProgress() - isn't the
former the superset of the latter?
{code}
conf.set(TezConfiguration.TEZ_AM_TEZ_UI_HISTORY_URL_TEMPLATE ,
TezConfiguration.TEZ_AM_TEZ_UI_HISTORY_URL_TEMPLATE_DEFAULT);
{code}
- is there a reason why this needs to be set in the test? Shouldn't the code
fall back to using the default if TEZ_AM_TEZ_UI_HISTORY_URL_TEMPLATE is not
set?
Patch also introduces new findbugs warnings.
> Job Tracking URL should point to the Tez UI
> -------------------------------------------
>
> Key: TEZ-2018
> URL: https://issues.apache.org/jira/browse/TEZ-2018
> Project: Apache Tez
> Issue Type: New Feature
> Reporter: Rohini Palaniswamy
> Assignee: Prakash Ramachandran
> Priority: Critical
> Attachments: TEZ-2018.1.patch, TEZ-2018.wip.1.patch
>
>
> We need to have the Job Tracking URL take users to the Tez UI if
> yarn.timeline-service.enabled=true
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
