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

Varun Vasudev commented on YARN-5600:
-------------------------------------

Thanks for the updated patch [~miklos.szeg...@cloudera.com]! Some more comments 
-

1)
{code}
+     * $LOG_DIRS
{code}
Wrong variable name 😃

2)
{code}
+    DELETE_DELAY("DELETE_DELAY");
{code}
How about DEBUG_DELETE_DELAY (for the variable and the environment variable) 
instead just to emphasize the debug part of it.

3)
bq. I removed -1 as the default value. However, the more changes and 
administrator needs to do to enable the feature, the less likely it will be 
used. Because of this I suggest using 10 minutes as a default. This allows the 
user to modify an application launch and copy the results, when needed without 
a reboot. Is 10 minutes acceptable in a production cluster?
No I think it’s fine for admins to be required to make the change. The reality 
is that 10 minutes may be acceptable to some and may not be acceptable to 
others.

4)
bq. Note: this change will break backward compatibility, although -1 was not 
documented before.
Given that it was undocumented, I think it’s fine to break it in trunk. Maybe 
in branch-2 we leave as it was but undocumented as earlier?

5)
{code}
+    <description>
+      Maximum mumber of seconds after an application finishes before the
+      nodemanager's DeletionService will delete the application's localized
+      file directory and log directory. This value is a security setting to
+      prevent unreliable or malicious clients keep files arbitrarily.
+
+      This is a maximum limit on the following values
+        yarn.nodemanager.delete.debug-delay-sec
+        Environment.DELETE_DELAY
+    </description>
+    <name>yarn.nodemanager.delete.max-debug-delay-sec</name>
+    <value>600</value>
+  </property>
{code}

This value shouldn’t affect yarn.nodemanager.delete.debug-delay-sec - it 
doesn’t make sense to have to modify a config file in two places if I want to 
enable debug delay for all applications. Instead of calling it 
"yarn.nodemanager.delete.max-debug-delay-sec”, how about calling it 
“yarn.nodemanager.delete.max-per-application-debug-delay-sec” that makes it 
clear that it’s related to the application debug feature and won’t conflict 
with "yarn.nodemanager.delete.max-debug-delay-sec"

6)
{code}
+  private int debugDelayDefaultMax;
{code}
Rename debugDelayDefaultMax to maxDebugDelay

7)
{code}
+  long getTaskCount() {
{code}
Annotate as VisibleForTesting?

> Add a parameter to ContainerLaunchContext to emulate 
> yarn.nodemanager.delete.debug-delay-sec on a per-application basis
> -----------------------------------------------------------------------------------------------------------------------
>
>                 Key: YARN-5600
>                 URL: https://issues.apache.org/jira/browse/YARN-5600
>             Project: Hadoop YARN
>          Issue Type: Improvement
>          Components: nodemanager
>    Affects Versions: 3.0.0-alpha1
>            Reporter: Daniel Templeton
>            Assignee: Miklos Szegedi
>              Labels: oct16-medium
>         Attachments: YARN-5600.000.patch, YARN-5600.001.patch, 
> YARN-5600.002.patch, YARN-5600.003.patch, YARN-5600.004.patch, 
> YARN-5600.005.patch, YARN-5600.006.patch, YARN-5600.007.patch, 
> YARN-5600.008.patch, YARN-5600.009.patch, YARN-5600.010.patch, 
> YARN-5600.011.patch
>
>
> To make debugging application launch failures simpler, I'd like to add a 
> parameter to the CLC to allow an application owner to request delayed 
> deletion of the application's launch artifacts.
> This JIRA solves largely the same problem as YARN-5599, but for cases where 
> ATS is not in use, e.g. branch-2.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org

Reply via email to