You are viewing a plain text version of this content. The canonical link for it is here.
Posted to yarn-issues@hadoop.apache.org by "Jian He (JIRA)" <ji...@apache.org> on 2017/05/22 21:01:04 UTC
[jira] [Comment Edited] (YARN-6613) Update json validation for new
native services providers
[ https://issues.apache.org/jira/browse/YARN-6613?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16020188#comment-16020188 ]
Jian He edited comment on YARN-6613 at 5/22/17 9:00 PM:
--------------------------------------------------------
Thanks Billie, patch looks good overall, some comments I had, most of them are cosmetic changes.
- remove the unused method AbstractClientProvider#processClientOperation ?
- ServiceApiUtil#validateConfigFile may be merged into this method {{validateConfigFiles(List<ConfigFile> configFiles, FileSystem fileSystem)}} in AbstractClientProvider ?
- create a common method for below method so that the code is not duplicated in the has-no-componet and has-component scenario.
{code}
AbstractClientProvider compClientProvider = SliderProviderFactory
.getClientProvider(comp.getArtifact());
compClientProvider.validateArtifact(comp.getArtifact(), fs
.getFileSystem());
// resource
if (comp.getResource() == null) {
comp.setResource(globalResource);
}
validateApplicationResource(comp.getResource(), comp);
// container count
if (comp.getNumberOfContainers() == null) {
comp.setNumberOfContainers(globalNumberOfContainers);
}
if (comp.getNumberOfContainers() == null
|| comp.getNumberOfContainers() < 0) {
throw new IllegalArgumentException(String.format(
RestApiErrorMessages.ERROR_CONTAINERS_COUNT_FOR_COMP_INVALID
+ ": " + comp.getNumberOfContainers(), comp.getName()));
}
validateConfigFile(comp.getConfiguration().getFiles(), fs
.getFileSystem());
compClientProvider.validateConfigFiles(comp.getConfiguration()
.getFiles(), fs.getFileSystem());
{code}
- validateApplicationPayload is a bit overloaded. maybe rename validateApplicationPayload to validateAndResolveApplicationPayload ?
- Could you add some comments to explain that this block of code is for resoliving external application's components or we may create a separate method for it, so that reader doesn't need to read through the code to understand what its doing.
{code}
for (Component comp : application.getComponents()) {
if (componentNames.contains(comp.getName())) {
throw new IllegalArgumentException("Component name collision: " +
comp.getName());
}
// artifact
if (comp.getArtifact() == null) {
comp.setArtifact(globalArtifact);
}
// configuration
comp.getConfiguration().mergeFrom(globalConf);
// If artifact is of type APPLICATION, read other application components
if (comp.getArtifact() != null && comp.getArtifact().getType() ==
Artifact.TypeEnum.APPLICATION) {
if (StringUtils.isEmpty(comp.getArtifact().getId())) {
throw new IllegalArgumentException(
RestApiErrorMessages.ERROR_ARTIFACT_ID_INVALID);
}
componentsToRemove.add(comp);
List<Component> applicationComponents = getApplicationComponents(fs,
comp.getArtifact().getId());
for (Component c : applicationComponents) {
if (componentNames.contains(c.getName())) {
// TODO allow name collisions? see AppState#roles
// TODO or add prefix to external component names?
throw new IllegalArgumentException("Component name collision: " +
c.getName());
}
componentNames.add(c.getName());
}
componentsToAdd.addAll(applicationComponents);
} else {
componentNames.add(comp.getName());
}
}
{code}
- Application configurations are merged into components before persisting, this will increase app json file size. For hdfs, it won't be a problem though. for zk that's relatively sensitive to file size, may be an issue. Any reason need to resolve it before persisting?
- In actionStart, why is it required to write back to hdfs?
{code}
// write app definition on to hdfs
persistApp(appDir, application);
{code}
- looks like SliderClient#monitorAppToState is only used by monitorAppToRunning ? we can just use monitorAppToRunning. no need to have this separate method.
- rename TestConfTreeLoadExamples to something else?
- TestMiscSliderUtils can be removed ? the methods (createAppInstanceTempPath, purgeAppInstanceTempFiles) for which it's testing seem only used by the test itself.
- rename ExampleConfResources to ExampleAppJson
- In Default and Tarball provider, only the the filename of the dest_file is used to crete the localized file, all parent paths are ignored which makes it confusing by user if user supplies with a full path. should we add additional validation that only filename should be used in the dest_file ? or make it create full path
was (Author: jianhe):
Thanks Billie, patch looks good overall, some comments I had, most of them are cosmetic changes.
- remove the unused method AbstractClientProvider#processClientOperation ?
- ServiceApiUtil#validateConfigFile may be merged into this method {{validateConfigFiles(List<ConfigFile> configFiles, FileSystem fileSystem)}} in AbstractClientProvider ?
- create a common method for below method so that the code is not duplicated in the has-no-componet and has-component scenario.
{code}
AbstractClientProvider compClientProvider = SliderProviderFactory
.getClientProvider(comp.getArtifact());
compClientProvider.validateArtifact(comp.getArtifact(), fs
.getFileSystem());
// resource
if (comp.getResource() == null) {
comp.setResource(globalResource);
}
validateApplicationResource(comp.getResource(), comp);
// container count
if (comp.getNumberOfContainers() == null) {
comp.setNumberOfContainers(globalNumberOfContainers);
}
if (comp.getNumberOfContainers() == null
|| comp.getNumberOfContainers() < 0) {
throw new IllegalArgumentException(String.format(
RestApiErrorMessages.ERROR_CONTAINERS_COUNT_FOR_COMP_INVALID
+ ": " + comp.getNumberOfContainers(), comp.getName()));
}
validateConfigFile(comp.getConfiguration().getFiles(), fs
.getFileSystem());
compClientProvider.validateConfigFiles(comp.getConfiguration()
.getFiles(), fs.getFileSystem());
{code}
- validateApplicationPayload is a bit overloaded. maybe rename validateApplicationPayload to validateAndResolveApplicationPayload ?
- Could you add some comments to explain that this block of code is for resoliving external application's components or we may create a separate method for it, so that reader doesn't need to read through the code to understand what its doing.
{code}
for (Component comp : application.getComponents()) {
if (componentNames.contains(comp.getName())) {
throw new IllegalArgumentException("Component name collision: " +
comp.getName());
}
// artifact
if (comp.getArtifact() == null) {
comp.setArtifact(globalArtifact);
}
// configuration
comp.getConfiguration().mergeFrom(globalConf);
// If artifact is of type APPLICATION, read other application components
if (comp.getArtifact() != null && comp.getArtifact().getType() ==
Artifact.TypeEnum.APPLICATION) {
if (StringUtils.isEmpty(comp.getArtifact().getId())) {
throw new IllegalArgumentException(
RestApiErrorMessages.ERROR_ARTIFACT_ID_INVALID);
}
componentsToRemove.add(comp);
List<Component> applicationComponents = getApplicationComponents(fs,
comp.getArtifact().getId());
for (Component c : applicationComponents) {
if (componentNames.contains(c.getName())) {
// TODO allow name collisions? see AppState#roles
// TODO or add prefix to external component names?
throw new IllegalArgumentException("Component name collision: " +
c.getName());
}
componentNames.add(c.getName());
}
componentsToAdd.addAll(applicationComponents);
} else {
componentNames.add(comp.getName());
}
}
{code}
- Application configurations are merged into components before persisting, this will increase app json file size. For hdfs, it won't be a problem though. for zk that's relatively sensitive to file size, may be an issue. Any reason need to resolve it before persisting changed?
- In actionStart, why is it required to write back to hdfs?
{code}
// write app definition on to hdfs
persistApp(appDir, application);
{code}
- looks like SliderClient#monitorAppToState is only used by monitorAppToRunning ? we can just use monitorAppToRunning. no need to have this separate method.
- rename TestConfTreeLoadExamples to something else?
- TestMiscSliderUtils can be removed ? the methods (createAppInstanceTempPath, purgeAppInstanceTempFiles) for which it's testing seem only used by the test itself.
- rename ExampleConfResources to ExampleAppJson
- In Default and Tarball provider, only the the filename of the dest_file is used to crete the localized file, all parent paths are ignored which makes it confusing by user if user supplies with a full path. should we add additional validation that only filename should be used in the dest_file ? or make it create full path
> Update json validation for new native services providers
> --------------------------------------------------------
>
> Key: YARN-6613
> URL: https://issues.apache.org/jira/browse/YARN-6613
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Billie Rinaldi
> Assignee: Billie Rinaldi
> Fix For: yarn-native-services
>
> Attachments: YARN-6613-yarn-native-services.001.patch, YARN-6613-yarn-native-services.002.patch, YARN-6613-yarn-native-services.003.patch, YARN-6613-yarn-native-services.004.patch
>
>
> YARN-6160 started some work enabling different validation for each native services provider. The validation done in ServiceApiUtil#validateApplicationPayload needs to updated accordingly. This validation should also be updated to handle the APPLICATION artifact type, which does not have an associated provider.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org