[jira] [Issue Comment Deleted] (OOZIE-3112) SparkConfigrationService overwrites properties provided via --properties-file option in SparkAction

["Attila Sasvari (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/OOZIE-3112?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Attila Sasvari updated OOZIE-3112:
----------------------------------
    Comment: was deleted

(was: Thanks [~gezapeti] for working on this.

Some comments:
- Use try with resources if possible
- Can you add a comment when print out the default properties file so that one can see the origin of the file? Something like "Generated by OozieSpark ActionExecutor", would help to review workflows.
{code}
#Generated by Oozie SparkActionExecutor
#Mon Nov 13 10:01:31 UTC 2017
test=42
{code} 

{{mergeAndAddPropetiesFile}} is a private method, either add proper javadoc comments to describe what it does or remove it entirely. I would suggest the latter. 
- nit: can you extract reads and writes to separate functions - it would help to understand what is going on (e.g. having processServerDefaultConfig, processLocalizedDefaultConfig, processUserProptiesFileConfig) 
- idea: if there is a collision / a config setting is overwritten during the process, it might help to log / notify the user about the decision taken. For example "Spark configuration \[XYZ] was present in files \[f1,f2,f3]...". In the "final" merged configuration file it is visible though...
- {{try (FileReader reader = new FileReader(file))}} could be {{try (Reader reader = new FileReader(file)) {}}
- Error messages are slightly inconsistent.
- {{"Cloud not process propagated Spark configs!" + e.getMessage())}} should be {{"Could not read propagated Spark config file [{1}]. Reason: "}} + use {{String.format() or something}}
- do not {{e.printStackTrace(System.err)}}
- {{System.err.println("Cloud not write out Spark config file!" + e.getMessage())}} should be {{System.err.println("Could not persist derived Spark config file. Reason:" + e.getMessage())}} or something
- do not {{e.printStackTrace(System.err)}}
- {{try (final FileWriter fileWriter}} should be {{try (final Writer fileWriter = }}
- {{Cloud not process Spark configs from file}} should be {{Could not read Spark config file [{1}]. Reason: " + e.getMessage()}} + use {{String.format() or something}} 
- do not {{e.printStackTrace(System.err)}}

{{TestSparkArgsExtractor.java}}
- Can you organize the code into "logical subsections" (setup, exercise, verification)? It is quite hard to see where the actual execution ({{final List<String> sparkArgs = new SparkArgsExtractor(actionConf).extract(mainArgs);}}) starts without vertical spaces.

General:
- Can you make references final and restrict visibility if possible. 
- Can you describe the new feature / behaviour in Oozie / Spark Action documentation ?
- Please file a ReviewBoard request next time. It would have been easier to review the changes.)

> SparkConfigrationService overwrites properties provided via --properties-file option in SparkAction
> ---------------------------------------------------------------------------------------------------
>
>                 Key: OOZIE-3112
>                 URL: https://issues.apache.org/jira/browse/OOZIE-3112
>             Project: Oozie
>          Issue Type: Bug
>            Reporter: Peter Cseh
>            Assignee: Peter Cseh
>         Attachments: OOZIE-3112.01.patch, OOZIE-3112.02.patch
>
>
> SparkConfigurationService injects the config values as --conf to SparkSubmit. This will overwrite propties provided in the --properies-file option which is not the expected behavior.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)