[
https://issues.apache.org/jira/browse/YARN-5610?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15456016#comment-15456016
]
Jian He commented on YARN-5610:
-------------------------------
Copy over comments from parent jira.
*API Models*
- {artifact, resource, launch_command, number_of_containers} in Application
seems duplicated with those inside the component. I feel in this scenario, a
default global setting for artifacts, launch_command etc. is not that
appropriate, different components may likely have different requirements.
IMHO, we only need the ones in Component, this makes the interface cleaner and
underlying implementation simpler?
- unique_component_support: what is the primary use-case to have distinct
component name ?
- What is the BaseResource object for? Why does Application, ApplicationStatus,
Container, Resource need to extend this class?
- What does the Artifact#APPLICATION mean ?
- ApplicationState: What is difference RUNNNG vs STARTED, FINISHED vs STOPPED
{code}
ACCEPTED, RUNNING, FINISHED, FAILED, STOPPED, STARTED;
{code}
- Application#lifetime: it is String type. Does this mean we have to define a
scheme for user to specify the time in string format? How about just using long
type ?
- ApplicationStatus#errorMessage, how about call it diagnostics ? sometimes we
may also return non-error messages.
*Implementation*
- “hadoop-yarn-services-api” should be under hadoop-yarn-slider module as peer
to hadoop-yarn-slider-core
- why the changes needed in hadoop-project/pom.xml
- We should not use a deprecated getPort() method {{logger.info("Listening at
port = {}", applicationApiServer.getPort());}}, jenkins will report error.
- couple of things for below code
{code}
HADOOP_CONFIG = getHadoopConfigs();
SLIDER_CONFIG = getSliderClientConfiguration();
{code}
-- We cannot load hdfs config, that's for hdfs servers. Any reason you need the
hdfs configs?
-- Instead of calling these two methods, I think we can just call
{{YarnConfiguration yarnConf = new YarnConfiguration()}}. This will
automatically load the yarn-site and core-site configs.
- Why do we need to explicitly call initHadoopBinding, which is already called
the super.init() previously.
{code}
SliderClient client = new SliderClient() {
@Override
public void init(org.apache.hadoop.conf.Configuration conf) {
super.init(conf);
try {
initHadoopBinding();
} catch (SliderException e) {
throw new RuntimeException(
"Unable to automatically init Hadoop binding", e);
} catch (IOException e) {
throw new RuntimeException(
"Unable to automatically init Hadoop binding", e);
}
}
};
{code}
- These two catch clauses are identical, and Exception extends Throwable, so we
only need catch Throwable, if that's desired.
{code}
} catch (Exception e) {
logger.error("Unable to create SliderClient", e);
throw new RuntimeException(e.getMessage(), e);
} catch (Throwable e) {
logger.error("Unable to create SliderClient", e);
throw new RuntimeException(e.getMessage(), e);
}
{code}
- This will never return null, because the numberOfContainers is intialized as
1. you might want to check zero ?
{code}
// container size
if (application.getNumberOfContainers() == null) {
throw new IllegalArgumentException(ERROR_CONTAINERS_COUNT_INVALID);
}
{code}
- The lifetime field will never be null, because it is intilized as
"unlimited" by default
{code}
// Application lifetime if not specified, is set to unlimited lifetime
if (application.getLifetime() == null) {
application.setLifetime(DEFAULT_UNLIMITED_LIFETIME);
}
{code}
- IIUC, all these code are not needed, because appOptions is only used for
logging, uniqueGlobalPropertyCache is not used logically, Python is not
required any more in yarn-slider
{code}
if (application.getConfiguration() != null
&& application.getConfiguration().getProperties() != null) {
for (Map.Entry<String, String> propEntry : application.getConfiguration()
.getProperties().entrySet()) {
if (PROPERTY_PYTHON_PATH.equals(propEntry.getKey())) {
addOptionsIfNotPresent(appOptions, uniqueGlobalPropertyCache,
SliderXmlConfKeys.PYTHON_EXECUTABLE_PATH, propEntry.getValue());
continue;
}
addOptionsIfNotPresent(appOptions, uniqueGlobalPropertyCache,
propEntry.getKey(), propEntry.getValue());
}
}
{code}
- In agent-less world, the status command is probably not required. We need a
different mechanism to determine container status. let's remove this for now
{code}
appConfOptTriples.addAll(Arrays.asList(compName, configPrefix.toLowerCase()
+ ".statusCommand", DEFAULT_STATUS_CMD));
{code}
- remove the unused parameter globalConf in createAppConfigComponent
- remove unused method createAppConfigGlobal
- remove the commented out code in this class, there are quite a few places
- Can you please explain what this code tries to accomplish ? it's hard to
understand what it tries to do.
{code}
List<String> convertedDeps = new ArrayList<>();
for (String dep : component.getDependencies()) {
if (compNameArtifactIdMap.containsKey(dep)) {
convertedDeps.add(compNameArtifactIdMap.get(dep));
} else {
convertedDeps.add(dep);
}
}
if (hasPropertyWithValue(component, PROPERTY_DNS_DEPENDENCY, "true")) {
if (component.getArtifact().getType() == Artifact.TypeEnum.APPLICATION) {
convertedDeps.add(component.getArtifact().getId());
} else {
convertedDeps.add(compName);
}
}
if (convertedDeps.size() > 0) {
appConfOptTriples.addAll(Arrays
.asList(compName, "requires", StringUtils.join(convertedDeps, ",")));
}
{code}
- The lifetime is tied to the application according to the API, why does it
need to pass down to every component? IIUC, the flow should be, once the app
timeouts, all components for this app timeout.
{code}
// set lifetime only to one component - chose whichever comes first
if (firstComponent) {
appCompOptionTriples.addAll(
createAppConfigComponent(comp.getName(), comp, comp.getName(),
lifetime, globalConfig, null, compNameArtifactIdMap));
firstComponent = false;
} else {
// TODO: Make lifetime non-null for now (avoid throwing exception)
appCompOptionTriples.addAll(
createAppConfigComponent(comp.getName(), comp, comp.getName(),
null, globalConfig, null, compNameArtifactIdMap));
}
{code}
> 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
