Mailing-List: contact yarn-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Date: Thu, 19 Jan 2017 22:27:27 +0000 (UTC)
From: "Jason Lowe (JIRA)" <jira@apache.org>
To: yarn-issues@hadoop.apache.org
Message-ID: <JIRA.13004600.1473776369000.56806.1484864847314@Atlassian.JIRA>
In-Reply-To: <JIRA.13004600.1473776369000@Atlassian.JIRA>
References: <JIRA.13004600.1473776369000@Atlassian.JIRA> <JIRA.13004600.1473776369914@jira-lw-us.apache.org>
Subject: [jira] [Commented] (YARN-5641) Localizer leaves behind tarballs
 after container is complete
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Thu, 19 Jan 2017 22:27:37 -0000


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

Jason Lowe commented on YARN-5641:
----------------------------------

Thanks for updating the patch!

I think getThread and the description is a little vague.  It would be more clear if the method were called something like getWaitingThread to indicate this is the thread waiting for the shell command to return, and the Javadoc should be more clear that it is the thread in the runCommand method or null if no thread is waiting for a shell command to complete.

runCommand needs to set the waiting thread field to null after the subprocess completes.

removeShell does not seem to be appropriate to be public or even necessary.  Shell instances should automatically remove themselves when the subprocess exits, and if the subprocess hasn't exited then they shouldn't be removed otherwise.  I don't think it's appropriate for the ContainerLocalizer to be removing shells, or am I missing something?

FSDownloadWrapper should be package-private.

HashSet is not thread safe, and multiple threads can be accessing it at the same time (e.g.: one thread in the executor pool may be trying to add itself to the set just as another is trying to call the set's contains method).  This needs to either be a synchronized set or a concurrent set.

Why doesn't the ContainerLocalizerWrapper constructor initialize the spylfs member?  It's odd to call the constructor then always have to poke fields in it.

To help reduce the changes in the patch, I'd recommend creating {{spylfs}}, {{random}}, and {{nmProxy}} local variables to eliminate all the changes that are simply adding the 'wrapper.' prefix.

Nit: Please use a much smaller check interval on the waitFor calls in the unit test.

Style Nit: Typically there is no space between the generic class name and the type specifier (e.g.: {{Map <>}} should be {{Map<>}})
Style Nit: Please do not use wildcard imports


> Localizer leaves behind tarballs after container is complete
> ------------------------------------------------------------
>
>                 Key: YARN-5641
>                 URL: https://issues.apache.org/jira/browse/YARN-5641
>             Project: Hadoop YARN
>          Issue Type: Bug
>            Reporter: Eric Badger
>            Assignee: Eric Badger
>         Attachments: YARN-5641.001.patch, YARN-5641.002.patch, YARN-5641.003.patch, YARN-5641.004.patch, YARN-5641.005.patch, YARN-5641.006.patch, YARN-5641.007.patch
>
>
> The localizer sometimes fails to clean up extracted tarballs leaving large footprints that persist on the nodes indefinitely. 


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

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