Rebase SparkSubmitTask onto ExternalProgramTask #1525
Conversation
|
The |
| @@ -189,12 +146,18 @@ def hadoop_conf_dir(self): | |||
| return configuration.get_config().get("spark", "hadoop-conf-dir", None) | |||
|
|
|||
| def get_environment(self): | |||
| env = os.environ.copy() | |||
| env = super(SparkSubmitTask, self).program_environment() | |||
There was a problem hiding this comment.
not a big fan of calling methods on the parent class. should only be doable for constructors
There was a problem hiding this comment.
Sure! It's just doing os.environ.copy() anyway. I'll change it.
|
@deuxpi Good idea. But It could be a nice new feature though, to add an option to turn it off? |
|
@ehdr Ack! Sorry, I meant |
Yes I think this is acceptable. :) |
|
I like this patch. I'm +1 the idea. @ehdr, When you think you've fixed all the comments from this PR, let us know and we'll merge. |
ehdr
force-pushed
the
rebase-spark-onto-external-program-task
branch
2 times, most recently
from
002d95b
to
db48a78
Compare
Add a property to ExternalProgramTask, which can be overridden to only log `stderr` when the program execution fails. The default remains to always log `stderr`, even if execution succeeds.
Seems to be an obsolete left-over from something.
ehdr
force-pushed
the
rebase-spark-onto-external-program-task
branch
from
db48a78
to
8d7876c
Compare
The public interface of `SparkSubmitTask` remains as-is. However, there will be subtle changes to the output to `stdout` and logs (e.g. 'Program failed[...]' with this patch vs. 'Spark job failed[...]' before). Also it will raise a `ExternalProgramRunError` on execution errors instead of a `SparkJobError` as before.
ehdr
force-pushed
the
rebase-spark-onto-external-program-task
branch
from
8d7876c
to
a0417c6
Compare
|
Great! All improvements are welcome and most code in this code base is not really elegant ^^ |
…-task Rebase SparkSubmitTask onto ExternalProgramTask
As a continuation of #1520, this is an example of how
SparkSubmitTaskcould be "rebased" ontoExternalProgramTask, since most of the code is shared between them. Not sure if this is a good way to go?The public interface of
SparkSubmitTaskremains as-is.However, there will be subtle changes to the output to
stdoutand logs (e.g. 'Program failed[...]' with this patch vs. 'Spark job failed[...]' before). Also it will raise aExternalProgramRunErroron execution errors instead of aSparkJobErroras before. Is this considered acceptable deviations from backwards compatibility?Finally, it will now log[changed back to preserve the old behavior in a later commit, thanks to comment from @deuxpi]stdoutandstderrwhenever it's (and only when it's) non-empty, even if the process exits successfully (return code 0). Previously,stderrwas only logged if the return code was non-zero.