[
https://issues.apache.org/jira/browse/TEZ-2226?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14503875#comment-14503875
]
Hitesh Shah commented on TEZ-2226:
----------------------------------
Comments:
Replace TezDomainException with something more generic like
HistoryACLPolicyException. Should be in same package as HistoryAclPolicyManager
" dag.setConf(TezConfiguration.TEZ_DAG_HISTORY_LOGGING,
Boolean.toString(false));" - can just use a string "false" instead?
{code}
426 LOG.warn("Turn Off ATS Logging for DAG " +
427 dag.getName() + " Due To Fail To Create Domain");
{code}
- Rephrase to ("Disabling history logging for dag " + dagName + " due to
error in setting up history acls", e);
- Domains and ATS should not be called as any history logger with its own
history acl manager could be used in the future.
- Please change other locations too. Also, please add "for session +
sessionName" in the case where the logging for full session is being disabled.
{code}
523 try {
524 if (dag == null) {
525 aclConfigs =
historyACLPolicyManager.setupSessionACLs(amConfig.getTezConfiguration(),
526 appId);
527 } else {
528 // Non-session mode
529 // As only a single DAG is support, we should combine AM and
DAG ACLs under the same
530 // acl management layer
531 aclConfigs =
historyACLPolicyManager.setupNonSessionACLs(amConfig.getTezConfiguration(),
532 appId, dag.getDagAccessControls());
533 dag.setConf(TezConfiguration.TEZ_DAG_HISTORY_LOGGING,
Boolean.toString(true));
534 }
535 } catch (TezDomainException e) {
536 boolean aclsEnabled =
amConfig.getTezConfiguration().getBoolean(TezConfiguration.TEZ_AM_ACLS_ENABLED,
537
TezConfiguration.TEZ_AM_ACLS_ENABLED_DEFAULT);
538 if (aclsEnabled) {
539 if (dag == null) {
540 LOG.warn("Turn Off ATS Logging Due To Fail To Create
Domain");
541
amConfig.getTezConfiguration().setBoolean(YarnConfiguration.TIMELINE_SERVICE_ENABLED,false);
542 } else {
543 LOG.warn("Turn Off ATS Logging for DAG " +
544 dag.getName() + " Due To Fail To Create Domain");
545 dag.setConf(TezConfiguration.TEZ_DAG_HISTORY_LOGGING,
Boolean.toString(false));
546 }
547 }
{code}
- code might be cleaner to have 2 separate catch blocks instead of one catch
with if/else.
{code}
*/
212 @ConfigurationScope(Scope.DAG)
213 public static final String TEZ_DAG_HISTORY_LOGGING = TEZ_PREFIX +
"dag.history.logging";
214 public static final boolean TEZ_DAG_HISTORY_LOGGING_DEFAULT = true;
{code}
- this should be marked private.
- class TezDomainException needs javadocs
{code}
boolean dagLogging =
newDAG.getConf().getBoolean(TezConfiguration.TEZ_DAG_HISTORY_LOGGING,
2031 TezConfiguration.TEZ_DAG_HISTORY_LOGGING_DEFAULT);
2032 historyEventHandler.setDagLoggingState(newDAG.getID().toString(),
dagLogging);
{code}
- why not pass in a flag as part of the DAGSubmittedEvent?
- this can be looked at and handled in the relevant history logging service.
See how pre-warm dags are handled in ATS logging.
{code}
throw new TezDomainException("Fail to create domain due to fail to post
domain");
{code}
- Change to "throw new TezDomainException("Fail to create domain due to fail
to post domain", e);
- the underlying root cause is getting lost.
For unit tests:
- one case is missing. Using the mock history acl manager before calling tez
session start and verifying that all dags submitted with this session have dag
logging disabled.
- also it would be good to add a mini cluster based test where when
setDagConf for logging false is done, the test checks that no data got pushed
to ATS. Likewise that not setting logging config via dag conf ( i.e. default
true ), the data gets pushed to ATS.
> Disable writing history to timeline if domain creation fails.
> -------------------------------------------------------------
>
> Key: TEZ-2226
> URL: https://issues.apache.org/jira/browse/TEZ-2226
> Project: Apache Tez
> Issue Type: Sub-task
> Reporter: Hitesh Shah
> Assignee: Chang Li
> Priority: Blocker
> Attachments: TEZ-2226.2.patch, TEZ-2226.3.patch, TEZ-2226.4.patch,
> TEZ-2226.5.patch, TEZ-2226.6.patch, TEZ-2226.7.patch, TEZ-2226.8.patch,
> TEZ-2226.patch, TEZ-2226.wip.2.patch, TEZ-2226.wip.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
