[jira] [Commented] (YARN-6366) Refactor the NodeManager DeletionService to support additional DeletionTask types.

["Varun Vasudev (JIRA)" <ji...@apache.org>]

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

Varun Vasudev commented on YARN-6366:
-------------------------------------

Thanks for the patch [~shanekumpf@gmail.com]! Some minor things to clean up -

1)
Should we add the 'user' field to the DeletionTask base class instead of keeping it in the FileDeletionTask class?

2)
{code}
if (proto.getTaskType() != null) {
{code}
Can you add a check for {code} proto.hasTaskType() {code} and then check if it's null?

3)
{code}
int taskId = proto.getId(); 
{code}
Shouldn't this be in the base converter? It seems to be a common piece of code that every task type will have to call.

4)
You don't need the ProtoToFileDeletionTaskConverter and DelegatingProtoToDeletionTaskConverter classes. Please move the convert functions to the ProtoUtils class as static functions.

5)
{code}
+    FileDeletionTask dependentDeletionTask = new FileDeletionTask(del, null,
+        userDirPath, new ArrayList<>());
{code}
Why create a new ArrayList here? You've used "null" in other places.

6)
The new tests you've added need to be renamed to match the naming convention. Invoked test functions need to be renamed to start with "test" (like testGetUser)

Thanks!

> Refactor the NodeManager DeletionService to support additional DeletionTask types.
> ----------------------------------------------------------------------------------
>
>                 Key: YARN-6366
>                 URL: https://issues.apache.org/jira/browse/YARN-6366
>             Project: Hadoop YARN
>          Issue Type: Bug
>          Components: nodemanager, yarn
>            Reporter: Shane Kumpf
>            Assignee: Shane Kumpf
>         Attachments: YARN-6366.001.patch, YARN-6366.002.patch, YARN-6366.003.patch, YARN-6366.004.patch, YARN-6366.005.patch, YARN-6366.006.patch
>
>
> The NodeManager's DeletionService only supports file based DeletionTask's. This makes sense as files (and directories) have been the primary concern for clean up to date. With the addition of the Docker container runtime, addition types of DeletionTask are likely to be required, such as deletion of docker container and images. See YARN-5366 and YARN-5670. This issue is to refactor the DeletionService to support additional DeletionTask's.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

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