Skip to content

Add concept of global resources which can be shared across applications#1

Open
lceleban-dd wants to merge 8 commits intomasterfrom
global_resources
Open

Add concept of global resources which can be shared across applications#1
lceleban-dd wants to merge 8 commits intomasterfrom
global_resources

Conversation

@lceleban-dd
Copy link

No description provided.

@@ -71,9 +82,7 @@ private[akkeeper] final class YarnLocalResourceManager(conf: Configuration,
}

def getExistingLocalResource(dstPath: Path): LocalResource = {
Copy link
Collaborator

@zak-dd zak-dd Dec 12, 2019

Choose a reason for hiding this comment

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

Both getExistingLocalResource and getExistingResource returns LocalResource. Could you please rename
getExistingLocalResource -> getLocalResourceFromStagingDir and
getExistingResource -> getLocalResource
?

Copy link
Author

Choose a reason for hiding this comment

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

done

val updatedUserConfig = args.userConfig.map(addStagingDirToUserConfig(_, baseStagingDir))
val updatedUserConfig = args.userConfig
.map(addStagingDirToUserConfig(_, baseStagingDir))
.map(addGlobalResourcesToUserConfig(_, args.globalResources))
Copy link
Collaborator

@zak-dd zak-dd Dec 12, 2019

Choose a reason for hiding this comment

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

I'm not sure why the staging directory configuration is not passed when a user config is missing. It was introduced fairly recently (10 months ago) in a PR named: "Fix usage of the default staging directory in case when a user that submitted application is different from a user that runs the application". I've asked Iaroslav this question.

Since MasterRunner fallbacks to ConfigFactory.load() anyway (https://github.com/ramencloud/akkeeper/blob/master/akkeeper/src/main/scala/akkeeper/master/MasterRunner.scala#L124), I'd suggest always sending the config:

val updatedUserConfig = args.userConfig.orElse(Some(ConfigFactory.empty()))
      .map(addStagingDirToUserConfig(_, baseStagingDir))
      .map(addGlobalResourcesToUserConfig(_, args.globalResources))

(something along theses lines) or putting global resources into a separate file (launcher config). I'm about to prepare a commit with a suggestion.

Copy link
Author

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to propose something along these lines: global_resources...ramencloud:global_resources_proposal

Copy link
Author

Choose a reason for hiding this comment

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

merged 👍

LocalResource.newInstance(
ConverterUtils.getYarnUrlFromURI(fs.makeQualified(status.getPath).toUri),
LocalResourceType.FILE, LocalResourceVisibility.APPLICATION,
ConverterUtils.getYarnUrlFromURI(fs. makeQualified(status.getPath).toUri),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Space

Copy link
Author

Choose a reason for hiding this comment

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

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants