Mailing-List: contact yarn-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Date: Mon, 22 May 2017 20:57:04 +0000 (UTC)
From: "Jian He (JIRA)" <jira@apache.org>
To: yarn-issues@hadoop.apache.org
Message-ID: <JIRA.13072604.1494970407000.266378.1495486624171@Atlassian.JIRA>
In-Reply-To: <JIRA.13072604.1494970407000@Atlassian.JIRA>
References: <JIRA.13072604.1494970407000@Atlassian.JIRA> <JIRA.13072604.1494970407994@jira-lw-us.apache.org>
Subject: [jira] [Commented] (YARN-6613) Update json validation for new
 native services providers
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Mon, 22 May 2017 20:57:10 -0000


    [ 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. 
{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