Skip to content
This repository has been archived by the owner on Aug 4, 2022. It is now read-only.

[REEF-1411] Support multiple RMs and other Hadoop distributions #1020

Closed
wants to merge 3 commits into from

Conversation

afchung
Copy link
Contributor

@afchung afchung commented Jun 1, 2016

This addressed the issue by

  • Reading RM web addresses from yarn-site.xml.
  • Deprecating MultipleRMUrlProvider in favor of YarnConfigurationUrlProvider.

JIRA:
REEF-1411

This addressed the issue by
  * Reading RM web addresses from yarn-site.xml.
  * Deprecating MultipleRMUrlProvider in favor of YarnConfigurationUrlProvider.

JIRA:
  [REEF-1411](https://issues.apache.org/jira/browse/REEF-1411)

namespace Org.Apache.REEF.Utilities
{
public static class Yarn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document.

@markusweimer
Copy link
Contributor

Would it make sense to create a mirror of the YARNConfiguration class from Java in C#? At least the fields we care about? We could even make it injectable and have accessor methods for the fields we need.

@afchung
Copy link
Contributor Author

afchung commented Jun 2, 2016

@markusweimer There is already a YarnClientConfiguration in C#, which I believe kind of parallels the Java YarnConfiguration. I've also addressed your CR comment. Please have another look, thanks!

@markusweimer
Copy link
Contributor

The YarnClientConfiguration we have is for REEF's YARN client, right? What I meant was to create a class that represents YARN's Java class called YarnConfiguration. It is instantiated off of the various .xml configuration files Hadoop uses and gives accessor methods (constants, really) to access the various keys in them.

@afchung
Copy link
Contributor Author

afchung commented Jun 3, 2016

@markusweimer I see. I think it's a good idea, but it's a much larger undertaking than what this PR calls for. Should we open a JIRA item for it?

@afchung
Copy link
Contributor Author

afchung commented Jun 3, 2016

By the way, I don't really like putting the YARN static class in Utilities since it's runtime dependent; however, creating a new project seems too aggressive. Do you think it's fine leaving it there for now? Maybe we can add an Unstable to it?

@markusweimer
Copy link
Contributor

it's a much larger undertaking than what this PR calls for. Should we open a JIRA item for it?

Agreed. I was just wondering whether we should start with the couple of configuration keys we need now here, and keep adding to them as we need more. I don't think it is wise for us to blindly create access methods for all configuration options without a clear use case.

/// <summary>
/// The Hadoop configuration directory environment variable.
/// </summary>
public const string HadoopConfDirEnvVariable = "HADOOP_CONF_DIR";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The const strings are dangerous when their value ever changes, right? I believe the C# compiler is free to inline them in client code of this. Maybe make them readonly instead?

@markusweimer
Copy link
Contributor

Done with a pass.

@afchung
Copy link
Contributor Author

afchung commented Jun 6, 2016

@markusweimer I've addressed your comments, please have another look. Thanks!

@markusweimer
Copy link
Contributor

LGTM. Will test and merge.

@markusweimer
Copy link
Contributor

@afchung I merged this but forgot the PR comment in the commit message. Please close.

@afchung afchung closed this Jun 7, 2016
@tcNickolas
Copy link
Member

AppVeyor reports test failure on O.A.R.Client.Tests.YarnConfigurationUrlProviderTests.UrlProviderReadsEnvVarConfiguredConfigFileAndParsesCorrectHttpUrl() for this PR: https://ci.appveyor.com/project/ApacheSoftwareFoundation/reef/build/587-master/tests

It reproduces on my local machine 100% of the runs. @afchung Please look into this failure with high priority.

@afchung
Copy link
Contributor Author

afchung commented Jun 7, 2016

@tcNickolas I've noticed. Opened #1036. Thanks!

@markusweimer
Copy link
Contributor

Sorry, this somehow slipped past me. Will test and merge #1036

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants