[SPARK-8657] [YARN] Fail to upload conf archive to viewfs

[litao-buptsse]

Fail to upload conf archive to viewfs in spark-1.4
JIRA Link: https://issues.apache.org/jira/browse/SPARK-8657

[AmplabJenkins]

Can one of the admins verify this patch?

[srowen]

This PR is vs branch-1.4, not master. You probably want to read https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark but please close this.

[litao-buptsse]

@srowen I sure want to push to branch-1.4, you mean my PR title format is not correct? Should it be like "[SPARK-8657] [YARN] Fail to upload conf archive to viewfs"?

[litao-buptsse]

@srowen you mean I should push this bug fix PR to master, not branch-1.4?

[litao-buptsse]

@srowen I found this bug appear only in branch-1.4 code, but not in master code. Should I reopen this issue?

[AmplabJenkins]

Can one of the admins verify this patch?

[srowen]

Hm, but that suggests something else resolved it. Can you locate the separate change that addressed this in master and see if and when it can be back-ported? you shouldn't try to separately merge the same change. If this is a duplicate of another PR then SPARK-8657 should be closed.

[litao-buptsse]

@srowen Yes, I see the related code has be deleted by this big issue. But the modify changes is still in master branch, not merge to branch-1.4. Will you bring there commits to branch-1.4 in the future?

Issue link: litao-buptsse@3811290#diff-b050df3f55b82065803d6e83453b9706L337

[srowen]

The code you are changing still exists in master, on about line 321. You would need to propose a change versus master; it would still need to be fixed there first right? I don't think that commit is actually related to this.

Is the better fix in ClientDistributedCacheManager.addResource? I don't see why it takes a FileSystem there, since it doesn't really make sense unless it matches the one used by its Path argument. If that can't be done for some reason I agree with this change, since I kind of looks like that was the intent in
50ab8a6
CC @vanzin for a thought

[vanzin]

Are you sure this doesn't happen on master? As Sean points out, the code there is the same.

I was trying to figure out what exactly is going on here; my hunch is that this is because copyFileToRemote calls makeQualified when returning the destination, and that changes the filesystem if the default fs is viewfs (so that it becomes a regular hdfs URL pointing directly at the right NN). Logging the paths before and after the call to copyFileToRemote would let us know if that's really what's going on.

That being said, the change looks correct, and probably ok for the 1.4 branch. On master it might be worth it to follow Sean's suggestion of simplifying ClientDistributedCacheManager.addResource.

[litao-buptsse]

@vanzin It was my mistake, I didn't check master code carefully. As you said, my PR can be accepted to branch-1.4?

[vanzin]

Yes, the change is fine for 1.4; I'd like to see a PR for master, though, so both branches don't get out of sync.

If you have the time to try, it would be really nice to see if my theory about what's going on is correct.

[litao-buptsse]

ok, I will try to finish a another PR for master.

[litao-buptsse]

@srowen @vanzin I also create a PR to simplify and correct method addResource() for master branch.
PR Link: #7053

[srowen]

Although the general process is to open a PR vs master and let the merger cherry-pick it, it's OK to open two PRs if you know that the code has diverged and the same change has to be applied differently in both places. I would prefer to make the same change in both places though as there is no compelling reason that the change has to work differently in master vs 1.4. If #7053 works, we can merge for master, and if you like, make a version of that for 1.4.

[SparkQA]

Test build #968 has started for PR 7042 at commit 18da350.

[SparkQA]

Test build #968 has finished for PR 7042 at commit 18da350.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[litao-buptsse]

Scala style tests failed. I will view my code again.

[litao-buptsse]

OK. I will try to make a version same as master #7053 for branch-1.4

[litao-buptsse]

I create a clean new PR #7055 (ajust scala code style) for this issue and I will close this PR. @srowen you can merge this PR to branch-1.4 temporaryly.
In future, I will create another PR like #7053 to simplify and correct method addResource() for master and branch-1.4 at the same time.(For #7053, some units can't pass, I will try to fix it)