[
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 [[email protected]]! 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: [email protected]
For additional commands, e-mail: [email protected]
