[jira] [Commented] (SLIDER-1107) Generate app configuration files in AM

["Gour Saha (JIRA)" <ji...@apache.org>]

    [ 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)