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-5610) Initial code for native services REST API
Date Thu, 01 Sep 2016 17:20:22 GMT

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

Jian He commented on YARN-5610:
-------------------------------

Some more comments:

- Application#get/setNumberOfContainers, this API is a bit inconsistent
-- when submit, it means the default number of containers for each component
-- when query app status, it means the total number of containers acroos all components.
I feel it's better to make it consistent, for the number of running containers,  user can
just infer it from the size of containers list or add a new filed called containers_running

- Question on the quickLinks API,  why does the component need to have quicklinks field? IIUC,
the quicklinks is mainly to show on the UI? In that case, having it in the application object
is enough?
- Can you upload the new Swagger sepcification ?
- Configuration object:
-- how are the properties filed passed to the container ?
- ConfigFile#dest_file (The absolute path that this configuration file should be mounted as)
  The user cannot assume an abritrary absolute path is valid in the remote container ? It
needs to be a relative path to the container local dir?
- ConfigFile#src_file: I think this could support all kinds of config files on HDFS, so that
user doesn't need to specify all configs in the request payload. e.g. user simply provide
a URI of the configFile on HDFS, YARN will localize the file for container to use 


- I dont see the place where queuName is set, also why is queueName set to label_expression
?
{code}
    if (queueName != null) {
      resCompOptTriples.addAll(Arrays.asList(compName,
          ResourceKeys.YARN_LABEL_EXPRESSION, queueName));
    }
{code}
- Given that APP_NAME is staic and specified by the user, why do we need to provide a place_holder
for App_name and then substitue? why can't user specify the app_name in the first place.
{code}
      Map<String, String> appQuicklinks = application.getQuicklinks();
      Map<String, String> placeholders = new HashMap<>();
      placeholders.put(PLACEHOLDER_APP_NAME, application.getName());
      if (appQuicklinks != null) {
        for (Map.Entry<String, String> quicklink : appQuicklinks.entrySet()) {
          JsonObject export = new JsonObject();
          export.addProperty("name", quicklink.getKey());
          export.addProperty("value",
              replacePlaceholders(quicklink.getValue(), placeholders));
          exportsArray.add(export);
        }
      }
{code}

- All these null checks are not needed, because they are validated upfront already
{code}
    if (comp.getArtifact() != null && comp.getArtifact().getType() != null
        && comp.getArtifact().getType() == Artifact.TypeEnum.DOCKER) {
{code}
similarly, 
{code}
          comp.getArtifact().getId() == null ? application.getArtifact()
              .getId() : comp.getArtifact().getId());
{code}
- invokeSliderClientRunnable: I don't quite understand why we need to set the setContextClassLoader
and then reset back, could you explain
- why is this code needed to be called in every rest API?
{code}
          File sliderJarFile = SliderUtils
              .findContainingJar(SliderClient.class);
          if (sliderJarFile != null) {
            logger.debug("slider.libdir={}", sliderJarFile.getParentFile()
                .getAbsolutePath());
            System.setProperty("slider.libdir", sliderJarFile.getParentFile()
                .getAbsolutePath());
          }
        } catch (Throwable t) {
          logger.warn("Unable to determine 'slider.libdir' path", t);
        }
{code}
- remove unused destroySliderClient method 
- deleteApplication API: listing all apps from RM is an expensive call.  Maybe we can directly
call kill/stop application and handle the Exception and return proper return-code.
{code}
    // Check if application exists in any state
    try {
      int applicationsFound = getSliderList(appName, false);
      if (applicationsFound < 0) {
        return Response.status(Status.NOT_FOUND).build();
      }
    } catch (Exception e) {
      logger.error("Delete application failed", e);
      return Response.status(Status.NOT_FOUND).build();
    }

    try {
      int livenessCheck = getSliderList(appName);
      if (livenessCheck == 0) {
        stopSliderApplication(appName);
        while (getSliderList(appName) == 0) {
          Thread.sleep(3000); // don't use thread sleep
        }
      }
{code}
- getApplication API: listing all apps is an expensive call in YARN. Instead We can handle
the ApplicationNotFoundExcetion from Yarn if the app does not exist.
{code}
    // Check if app exists
    try {
      int livenessCheck = getSliderList(appName);
      if (livenessCheck < 0) {
        logger.info("Application not running");
        ApplicationStatus applicationStatus = new ApplicationStatus();
        applicationStatus.setErrorMessage(ERROR_APPLICATION_NOT_RUNNING);
        applicationStatus.setCode(ERROR_CODE_APP_IS_NOT_RUNNING);
        return Response.status(Status.NOT_FOUND).entity(applicationStatus)
            .build();
      }
{code}

- getSliderApplicationStatus, getSliderApplicationRegistry: I wonder why the corresponding
implementation in slider: SliderClient#actionStatus, actionRegistry need to write the output
into a file and then let caller read the file. why not returnning the output directly to the
caller.

- This code is not needed, it can never happen.
{code}
    // ensure that the application name from the status report matches the
    // requested application name
    if (StringUtils.isEmpty(appName)
        || !app.getName().equals(appName)) {
      return Response.status(Status.NOT_FOUND).build();
    }
{code}
- This code seems not needed, as artifact is used nowhere, we can remove
{code}
    // application info
    if (applicationRoles != null && !componentNames.isEmpty()) {
      JsonObject applicationRole = jsonGetAsObject(applicationRoles,
          componentNames.get(0));
      if (applicationRole != null) {
        Artifact artifact = new Artifact();
        // how to get artifact id - docker image name??
        artifact.setId(null);
      }
    }
{code}
- Again, may be we can just call stop to save the getSliderList call. 
{code}
        int livenessCheck = getSliderList(appName);
        if (livenessCheck == 0) {
          return stopSliderApplication(appName);
{code}
- The RestApi interface is empty,  may be we can remove ?

> Initial code for native services REST API
> -----------------------------------------
>
>                 Key: YARN-5610
>                 URL: https://issues.apache.org/jira/browse/YARN-5610
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Gour Saha
>         Attachments: YARN-4793-yarn-native-services.001.patch
>
>
> This task will be used to submit and review patches for the initial code drop for the
native services REST API 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

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


Mime
View raw message