[
https://issues.apache.org/jira/browse/SLIDER-1107?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15405628#comment-15405628
]
Gour Saha commented on SLIDER-1107:
-----------------------------------
[~billie.rinaldi] I reviewed all the commits in the branch
feature/SLIDER-1107_AM_config_generation and overall everything looks good. Few
comments -
+*slider-core/src/main/java/org/apache/slider/common/params/ActionResourceArgs.java*+
{quote}
@Parameter(names = \{ARG_RESOURCE\},
description = "Name of the file or directory")
public String resource;
{quote}
This will result in the cmd to be something like "slider resource --resource
<>". Do you think we should name it to something like --source (or something
more appropriate)? It goes hand in hand with --destdir. What do you think?
+*slider-core/src/main/java/org/apache/slider/common/params/SliderActions.java*+
bq. String DESCRIBE_ACTION_RESOURCE = "Manage a file (install, delete, list)
in the sub-folder 'resources' of the user's Slider base directory";
Should we change to (only the _*resources*_ word was moved around)?
{code}
String DESCRIBE_ACTION_RESOURCE = "Manage a file (install, delete, list) in
the 'resources' sub-folder of the user's Slider base directory";
{code}
Also, we should create a _*--help*_ option (like other commands like create
has) such that {{./slider resource --help}} provides usage details for resource
command. This let’s users see the help even when run without connection to a
valid RM.
+*slider-core/src/main/java/org/apache/slider/common/tools/CoreFileSystem.java*+
In method
bq. public String cat(Path path) throws IOException \{
{code}
FSDataInputStream in = fileSystem.open(path);
int count = in.read(b);
{code}
Input stream is not closed.
+*slider-core/src/main/java/org/apache/slider/core/conf/ConfTreeOperations.java*+
bq. public static ConfTreeOperations fromString(String json) throws
Should we call the method _*fromJson*_?
+*slider-core/src/main/java/org/apache/slider/core/launch/AbstractLauncher.java*+
bq. public void addLocalResource(String subpath, LocalResource resource,
String mountPath) {
Make _*subpath*_ camel case to _*subPath*_, and similar change in the other
overloaded method also.
+*slider-core/src/main/java/org/apache/slider/providers/agent/AgentClientProvider.java*+
bq. metaInfo = new MetainfoParser().fromJsonStream(new
FileInputStream(filePath));
I think the input stream is not closed in fromJsonStream like fromXmlStream
closes. Can you please verify?
bq. String client_script = null;
Camel case it to clientScript
bq. if (packages != null && packages.size() > 0)
packages won’t be null so no need to check for != null
{quote}
424: e.printStackTrace();
432: e.printStackTrace();
458: e.printStackTrace();
{quote}
Should we just use logger to log some pretty msgs here (if intention is to
ignore the exceptions) or throw BadConfigException?
bq. 435: if (config != null) {
Was intent here to check if (clientRoot == null && config != null) ?
+*slider-core/src/main/java/org/apache/slider/providers/agent/AgentProviderService.java*+
bq. private static synchronized Path uploadResource(File resource,
Why is this method synchronized and static?
{quote}
1365: if (component != null && component.getCategory().equals("CLIENT")) {
{quote}
I don’t think component will be null here.
bq. 3025: finished = false;
I am not sure I understand what happens when _*finished*_ is set to _false_ and
the while loop goes over again. Can you explain?
+*slider-core/src/main/java/org/apache/slider/providers/agent/application/metadata/MetainfoParser.java*+
{quote}
78: digester.addObjectCreate("*/osSpecific/packages/package",
OSPackage.class);
79: digester.addBeanPropertySetter("*/osSpecific/packages/package/type");
80: digester.addBeanPropertySetter("*/osSpecific/packages/package/name");
81: digester.addSetNext("*/osSpecific/packages/package", "addOSPackage");
{quote}
Can these cause backward compatibility issues?
> Generate app configuration files in AM
> --------------------------------------
>
> Key: SLIDER-1107
> URL: https://issues.apache.org/jira/browse/SLIDER-1107
> Project: Slider
> Issue Type: Bug
> Reporter: Billie Rinaldi
> Assignee: Billie Rinaldi
> Fix For: Slider 1.0.0
>
>
> Currently, each container generates its own application configuration files.
> Instead, we could do this in the AM and have YARN localize the configuration
> files. Having some basic config generation in the AM may allow us to
> simplify the config generation code in the app packages. Also, it would be
> much better in the case of Docker containers, where we would prefer not to
> have to execute our own code inside the container.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
