[
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-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
