[
https://issues.apache.org/jira/browse/MAPREDUCE-5224?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13665677#comment-13665677
]
Ivan Mitic commented on MAPREDUCE-5224:
---------------------------------------
Thanks Xi, you're almost there! A few additional comments below. Once you
address those, +1 from me
1. You'll have to impersonate the MR owner when you're querying for the
systemDirFs (same routine as with defaultFs). Sorry, I missed this when I
initially reviewed the patch.
After:
{code}
if (defaultFs == null) {
defaultFs = mrOwner.doAs(new PrivilegedExceptionAction<FileSystem>() {
public FileSystem run() throws IOException {
return FileSystem.get(conf);
}});
}
{code}
add the following:
{code}
if (systemDirFs == null) {
systemDirFs = mrOwner.doAs(new
PrivilegedExceptionAction<FileSystem>() {
public FileSystem run() throws IOException {
Path sysDir = new Path(conf.get("mapred.system.dir",
"/tmp/hadoop/mapred/system"));
return FileSystem.get(sysDir.toUri(), conf);
}});
}
{code}
Once you implement above you should be able to simplify getSystemDir() by
assuming that systemDirFs is different than null. This will allow you to get
rid of the IOException (#2 comment from my initial review).
2. Nit: JobTracker.java: It seems that a tab slipped in:
{code}
if (systemDirFs.exists(restartFile)) {
systemDirFs.delete(tmpRestartFile, false); // delete the tmp file
} else if (systemDirFs.exists(tmpRestartFile)) {
{code}
I also see some invalid indentation:
{code}
// disable recovery if this is a restart
shouldRecover = false;
{code}
Please correct.
3. Please remove try/catch from TestJobTrackerWithNonDefaultFS#tearDown since
it can possibly mask a problem. Instead you can add IOException to the throws
clause of the method:
{code}
public void tearDown() throws IOException {
{code}
4. TestJobTrackerwithNonDefaultFs#testSystemDir: No need for the try/catch
block in the test, please remove. The test will fail if any of its asserts fail.
5. Can you also please change TestJobTrackerWithNonDefaultFS#MAPRED_SYS_DIR to
the following:
{code}
private final String MAPRED_SYS_DIR =
System.getProperty("test.build.data", "/tmp") + "/mapred/system";
{code}
Guideline is for all local test files to go under test.build.data folder.
6. Nit: TestJobTrackerwithNonDefaultFs: You can use assertTrue instead:
{code}
assertEquals("Check if the system dir exists ",
FileSystem.get(sysDirPathURL, conf).exists(sysDirPath), true);
{code}
Btw, when you’re using assertEquals, you should place the expected value as the
first arg, and the test value as the second arg. For example,
assertEquals(true, fs.get().exists()).
> JobTracker should allow the system directory to be in non-default FS
> --------------------------------------------------------------------
>
> Key: MAPREDUCE-5224
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-5224
> Project: Hadoop Map/Reduce
> Issue Type: Bug
> Components: jobtracker
> Reporter: Xi Fang
> Assignee: Xi Fang
> Priority: Minor
> Fix For: 1-win
>
> Attachments: MAPREDUCE-5224.2.patch, MAPREDUCE-5224.3.patch,
> MAPREDUCE-5224.patch
>
>
> JobTracker today expects the system directory to be in the default file
> system
> if (fs == null) {
> fs = mrOwner.doAs(new PrivilegedExceptionAction<FileSystem>() {
> public FileSystem run() throws IOException {
> return FileSystem.get(conf);
> }});
> }
> ...
> public String getSystemDir() {
> Path sysDir = new Path(conf.get("mapred.system.dir",
> "/tmp/hadoop/mapred/system"));
> return fs.makeQualified(sysDir).toString();
> }
> In Cloud like Azure the default file system is set as ASV (Windows Azure Blob
> Storage), but we would still like the system directory to be in DFS. We
> should change JobTracker to allow that.
--
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
