[
https://issues.apache.org/jira/browse/MAPREDUCE-6052?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14191437#comment-14191437
]
Zhijie Shen commented on MAPREDUCE-6052:
----------------------------------------
[~djp]. thanks for the patch. In general, the patch looks fine and implements
Vinod's proposal. Some comments.
1. The method has javadoc. It's good to add one line for "conf".
{code}
public static void addLog4jSystemProperties(
{code}
2. Unnecessary import in JobSubmitter
3. It seems that ClientDistributedCacheManager blah blah doesn't need to be
moved. We can invoke addLog4jToDistributedCache inside copyAndConfigureFiles.
{code}
addLog4jToDistributedCache(job, submitJobDir);
// set the timestamps of the archives and files
// set the public/private visibility of the archives and files
ClientDistributedCacheManager.determineTimestampsAndCacheVisibilities(conf);
// get DelegationToken for cached file
ClientDistributedCacheManager.getDelegationTokens(conf, job
.getCredentials());
{code}
4. Return null too? Otherwise, mapreduce.job.log4j-properties-file = "" will
result in exception?
{code}
if (file.isEmpty()) {
throw new IllegalArgumentException("File name can't be empty string");
}
{code}
5. The comment is staled?
{code}
// first copy local log4j.properties file to jobtrackers filesystem
{code}
6. It seem not to be necessary because submitJobDir should have been created in
copyAndConfigureFiles
{code}
LOG.debug("default FileSystem: " + jtFs.getUri());
FsPermission mapredSysPerms =
new FsPermission(JobSubmissionFiles.JOB_DIR_PERMISSION);
if (!jtFs.exists(submitJobDir)) {
submitJobDir = jtFs.makeQualified(submitJobDir);
submitJobDir = new Path(submitJobDir.toUri().getPath());
FileSystem.mkdirs(jtFs, submitJobDir, mapredSysPerms);
}
{code}
BTW, would you please comment how the patch works if you've conducted
end-to-end local test?
> Support overriding log4j.properties per job
> -------------------------------------------
>
> Key: MAPREDUCE-6052
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-6052
> Project: Hadoop Map/Reduce
> Issue Type: Bug
> Affects Versions: 2.5.0
> Reporter: Junping Du
> Assignee: Junping Du
> Attachments: MAPREDUCE-6052-v2.patch, MAPREDUCE-6052-v3.patch,
> MAPREDUCE-6052.patch
>
>
> For current MR application, the "log4j.configuration" is hard coded to
> container-log4j.properties within each node. We still need flexibility to
> override it per job like what we do in MRV1.
> {code}
> public static void addLog4jSystemProperties(
> String logLevel, long logSize, int numBackups, List<String> vargs) {
> vargs.add("-Dlog4j.configuration=container-log4j.properties");
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
