[
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: [email protected]
For additional commands, e-mail: [email protected]
