hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jian He (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-6613) Update json validation for new native services providers
Date Mon, 22 May 2017 20:57:04 GMT

    [ https://issues.apache.org/jira/browse/YARN-6613?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16020188#comment-16020188

Jian He commented on YARN-6613:

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. 
      AbstractClientProvider compClientProvider = SliderProviderFactory
      compClientProvider.validateArtifact(comp.getArtifact(), fs

      // resource
      if (comp.getResource() == null) {
      validateApplicationResource(comp.getResource(), comp);

      // container count
      if (comp.getNumberOfContainers() == null) {
      if (comp.getNumberOfContainers() == null
          || comp.getNumberOfContainers() < 0) {
        throw new IllegalArgumentException(String.format(
                + ": " + comp.getNumberOfContainers(), comp.getName()));
      validateConfigFile(comp.getConfiguration().getFiles(), fs
          .getFiles(), fs.getFileSystem());
- 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.
    for (Component comp : application.getComponents()) {
      if (componentNames.contains(comp.getName())) {
        throw new IllegalArgumentException("Component name collision: " +
      // artifact
      if (comp.getArtifact() == null) {
      // configuration
      // 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(
        List<Component> applicationComponents = getApplicationComponents(fs,
        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: " +
      } else {
- 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?
   // write app definition on to hdfs
    persistApp(appDir, application);
- 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

To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org

View raw message