[REEF-899] Make JobSubmissionDirectory in REEF-Runtime-YARN

[dongjoon-hyun]

According to the package name of JobSubmissionDirectory,
'org.apache.reef.driver.parameters', it is more natural that
JobSubmissionDirectory is in REEF-Common module with other parameters
having the same package name together.

JIRA:
REEF-899

Pull request:
This closes #

[markusweimer]

I'm not sure about this change. This parameter only makes sense on the YARN runtime, after all. It is in the wrong package, though.

[dongjoon-hyun]

Oh, then, may I change that package name according to the directory path like the following?

package org.apache.reef.runtime.yarn.driver.parameters;

I guessed that the reason of package naming was both reef-runtime-yarn and reef-runtime-hdinsight use that.

[markusweimer]

This is tricky, as the class is public. We might have to do a whole round of deprecation / renaming.

[dongjoon-hyun]

I see.

[dongjoon-hyun]

@markusweimer , I'm preparing a whole round of deprecation / renaming by maintaining two files in their own correct packages.

• org.apache.reef.driver.parameters.JobSubmissionDirectory (@deprecated in 0.14)
• org.apache.reef.runtime.yarn.driver.parameters.JobSubmissionDirectory (for 0.14 and so on)

By the way, it seems to be really tricky in two places.

• First, we need to bind both classes simultaneously in a single static YarnDriverConfiguration.CONF. Is it possible use one JOB_SUBMISSION_DIRECTORY for both classes? Otherwise, we should make another YARN_JOB_SUMBISSION_DIRECTORY and change all the code with .set(YarnDriverConfiguration.JOB_SUBMISSION_DIRECTORY, jobSubmissionFolder)

public class YarnDriverConfiguration extends ConfigurationModuleBuilder {
  public static final ConfigurationModule CONF = new YarnDriverConfiguration()
    .bindNamedParameter(org.apache.reef.driver.parameters.JobSubmissionDirectory.class, JOB_SUBMISSION_DIRECTORY)
     .bindNamedParameter(org.apache.reef.runtime.yarn.driver.parameters.JobSubmissionDirectory.class, JOB_SUBMISSION_DIRECTORY)

• Second, in Tang injection, we can not declare two different injectors distinguished by @Parameter annotation. How can we handle this in Tang world elegantly? Could you give some advice for this kind of transition?

/**
 * @deprecated in 0.14
 */
@Inject
UploaderToJobFolder(@Parameter(org.apache.reef.driver.parameters.JobSubmissionDirectory.class) final String jobSubmissionDirectory,
                    final YarnConfiguration yarnConfiguration) throws IOException {
  this.jobSubmissionDirectory = jobSubmissionDirectory;
  this.fileSystem = FileSystem.get(yarnConfiguration);
}

@Inject
UploaderToJobFolder(@Parameter(org.apache.reef.runtime.yarn.driver.parameters.JobSubmissionDirectory.class) final String jobSubmissionDirectory,
                      final YarnConfiguration yarnConfiguration) throws IOException {
  this.jobSubmissionDirectory = jobSubmissionDirectory;
  this.fileSystem = FileSystem.get(yarnConfiguration);
}

[markusweimer]

This is tricky indeed. Maybe we need to deprecate it in this version, and make it private in the next? People shouldn't bind to it directly anyhow.

[dongjoon-hyun]

Thank you! I will do like that by splitting this issue into a few sub-issues.

[dongjoon-hyun]

Sorry, @markusweimer . I was confused. UploaderToJobFolder is not public class. So, we can change safely. I think I've finish this issue by doing the followings.

1. I duplicated JobSubmissionDirectory.java in both packages.
2. Mark the one in wrong package as @deprecated.
3. Change non-public methods
4. Bind both legacy and new one in YarnDriverConfiguration and HDInsightDriverConfiguration and make a TODO on that.

If I missed something, please let me know.

[markusweimer]

Please move the TODO into a // comment. I don't think we have a style guide for this, but this is how we always do it elsewhere.And I fully expect this to be missed if we ever write a script to find them :-)

[markusweimer]

This could work. However, we need to validate on an actual cluster to be sure this doesn't break anything. Can you test on Linux/YARN?

[dongjoon-hyun]

Yes. It passed

$ ./bin/runyarntests.sh
...
Time: 373.096

OK (29 tests)

Actually, I used a docker-based Hadoop cluster (1 name node + 3 data node) for development.
(Hadoop 2.7.1 and JDK 7 on Linux)
I think that is enough for testing.

[bgchun]

@dongjoon-hyun @markusweimer Just to be safe, it'd be also good to test on HDInsight.

[dongjoon-hyun]

+1 for the idea, @bgchun . But, I'm wondering how to do by myself?
I found the following link, but it's a one week trial. We need some sustainable way.
https://azure.microsoft.com/en-us/blog/try-hadoop-on-azure-hdinsight-7/

[bgchun]

@markusweimer @afchung Suggestion?

[afchung]

@bgchun @dongjoon-hyun We have access to HDInsight clusters. I'll test and merge this PR if it passes. Thanks!

[dongjoon-hyun]

Thank you, @afchung .

[markusweimer]

@afchung Can you please also check a .NET app? This might have implications for those as well.

[afchung]

@markusweimer @dongjoon-hyun Ran C# and Java jobs on HDInsight and local, and everything has passed. Thanks!

[dongjoon-hyun]

Thank you for testing, @afchung !