tez-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hitesh Shah (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (TEZ-2018) Job Tracking and History URL should point to the Tez UI
Date Tue, 03 Feb 2015 22:59:34 GMT

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

Hitesh Shah edited comment on TEZ-2018 at 2/3/15 10:59 PM:
-----------------------------------------------------------

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? Why is it false by default? 

  - 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. 



 











was (Author: hitesh):
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 and History 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)

Mime
View raw message