[
https://issues.apache.org/jira/browse/YARN-661?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13705393#comment-13705393
]
Omkar Vinit Joshi commented on YARN-661:
----------------------------------------
Thanks [~zjshen] .. uploading updated patch..
bq. Not sure getCancelIfAnyDependentTaskFailedFlag is necessary. If deletion of
the sub-dir fails, should deletion of the root-dir fail automatically?
In this scenario we want to cancel this but may not be the case always. I will
remove it but that would have been an added control for user.
bq. Instead of synchronizing over HashMap, how about using ConcurrentHashMap
instead?
yeah fixed it..
{code}
+ List<FileDeletion> dependentTaskList =
+ this.deletionTaskDependencyMap.putIfAbsent(
+ parentTask, new ArrayList<FileDeletion>());
{code}
there is a problem with this as we end up creating new objects for every call.
It is present at lot of places in yarn and need to be fixed. the containsKey
check even though looks complicated avoids extra object creation on heap.
bq. WRT the dependency, I'd like rather call task pair predecessor and
successor instead of parent and child, because it's a dag not a tree. Moreover,
how about defining public void
populateFileDeletionTaskDependency(List<FileDeletion>, FileDeletion), and
populateFileDeletionTaskDependency(List<FileDeletion>, List<FileDeletion>)
wraps it. In fact, the former one seems to be enough for the patch.
yeah renaming parent -> predecessor and child -> successor. I think current
(List<>, List<>) api is more flexible and we can keep it that way.
bq. How about calling it delete as well, just overloading the method?
yeah wanted to overload only but is not helping me in junit tests.
MockitTo.verify is not smart enough to distinguish between overloaded
methods...hence giving different name...
{code}
verify(delService, times(1)).deleteHelper(
argThat(new FileDeletionInclude(user, null,
new String[] { destinationFile })));
verify(delService, times(1)).deleteHelper(
argThat(new FileDeletionInclude(null, ContainerLocalizer.USERCACHE
+ "_DEL_", new String[] {})));
{code}
bq. There's one finbugs warning to fix.
Hopefully fixed it.. I think it is complaining because I am exposing final
array. This too I need it for testing. Let me see if findbug complains again
then will fix it in some other way.
bq. Please use LocalResource.newInstance instead.
not related to this patch ..was just reformatting..but fixed it ..trivial fix.
bq. How about refactoring the code in the following pattern, which should be
more clear.
makes sense... fixed it..
> NM fails to cleanup local directories for users
> -----------------------------------------------
>
> Key: YARN-661
> URL: https://issues.apache.org/jira/browse/YARN-661
> Project: Hadoop YARN
> Issue Type: Bug
> Components: nodemanager
> Affects Versions: 2.1.0-beta, 0.23.8
> Reporter: Jason Lowe
> Assignee: Omkar Vinit Joshi
> Attachments: YARN-661-20130701.patch, YARN-661-20130708.patch
>
>
> YARN-71 added deletion of local directories on startup, but in practice it
> fails to delete the directories because of permission problems. The
> top-level usercache directory is owned by the user but is in a directory that
> is not writable by the user. Therefore the deletion of the user's usercache
> directory, as the user, fails due to lack of permissions.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
