Mailing-List: contact mapreduce-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: mapreduce-issues@hadoop.apache.org
Date: Fri, 13 Feb 2015 01:22:12 +0000 (UTC)
From: "Karthik Kambatla (JIRA)" <jira@apache.org>
To: mapreduce-issues@hadoop.apache.org
Message-ID: <JIRA.12724549.1404162748000.34013.1423790532334@Atlassian.JIRA>
In-Reply-To: <JIRA.12724549.1404162748000@Atlassian.JIRA>
References: <JIRA.12724549.1404162748000@Atlassian.JIRA>
 <JIRA.12724549.1404162748627@arcas>
Subject: [jira] [Commented] (MAPREDUCE-5951) Add support for the YARN Shared
 Cache
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable


    [ https://issues.apache.org/jira/browse/MAPREDUCE-5951?page=3Dcom.atlas=
sian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=3D=
14319375#comment-14319375 ]=20

Karthik Kambatla commented on MAPREDUCE-5951:
---------------------------------------------

Sorry for the delay in getting to this. Getting a continuous chunk of time =
to look at this somewhat large patch was hard.=20

Here are my first round of comments - a combination of high-level and detai=
led comments. Let us see if we can get some of this in through other JIRAs =
first, to allow for a more thorough review.
# DistributedCache changes aren=E2=80=99t central to what this JIRA is tryi=
ng to address. Could we leave them out and address in another JIRA?=20
## This has nothing to do with this patch, but it would be nice to make the=
 code around setting CLASSPATH_FILES a little more readable. Could we defin=
e another String prefix to hold =E2=80=9C=E2=80=9D or classpath, based on w=
hether classpath is null.=20
# Job
## The new APIs should all be @Unstable
## Let us make the javadoc for the new APIs a little more formal - we don=
=E2=80=99t need to mention SCMClientProtocol.use, or that the APIs are inte=
nded for user use. Even for the return value, I would go with something lik=
e =E2=80=9CIf shared cache is enabled and the resource added successfully, =
return true. Otherwise, return false.=E2=80=9D
## How about renaming the methods to addFileToSharedCache, addArchiveToShar=
edCache, addFileToSharedCacheAndClasspath?=20
## Make both new methods private static instead of static private.
# JobID changes might not be required. Use ConverterUtils#toApplicationId?=
=20
# JobImpl
## cleanupSharedCacheResources - nit: I would check for (checksums =3D=3D n=
ull || checksums.length =3D=3D 0) and return to save on indentations. 80 ch=
ars is already too small.
## cleanupSharedCacheUploadPolicies - javadoc should use block comments. We=
ll, may be a nit.
# JobSubmitter
## Can we do the code moving from JobSumitter to FileUploader (may be, we n=
eed a more descriptive name) to another JIRA and look at that first if need=
ed. Otherwise, it is hard to review the changes.
## May be, I am misreading the patch. Is this patch hardcoding MR job submi=
ssion to always use SharedCache? If yes, we should definitely avoid that.=
=20
# mapred-default.xml: We need a little more fool-proof config. The way the =
patch currently is, a typo will lead to unexpected behavior without any war=
nings.


> Add support for the YARN Shared Cache
> -------------------------------------
>
>                 Key: MAPREDUCE-5951
>                 URL: https://issues.apache.org/jira/browse/MAPREDUCE-5951
>             Project: Hadoop Map/Reduce
>          Issue Type: New Feature
>            Reporter: Chris Trezzo
>            Assignee: Chris Trezzo
>         Attachments: MAPREDUCE-5951-trunk-v1.patch, MAPREDUCE-5951-trunk-=
v2.patch, MAPREDUCE-5951-trunk-v3.patch, MAPREDUCE-5951-trunk-v4.patch, MAP=
REDUCE-5951-trunk-v5.patch, MAPREDUCE-5951-trunk-v6.patch
>
>
> Implement the necessary changes so that the MapReduce application can lev=
erage the new YARN shared cache (i.e. YARN-1492).
> Specifically, allow per-job configuration so that MapReduce jobs can spec=
ify which set of resources they would like to cache (i.e. jobjar, libjars, =
archives, files).


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