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