-
Notifications
You must be signed in to change notification settings - Fork 66
Feature: Spark add output logs flag #468
Feature: Spark add output logs flag #468
Conversation
@@ -1,3 +1,4 @@ | |||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: alpha sort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same in other files.
spinner.start() | ||
with open(os.path.abspath(os.path.expanduser(args.output)), "w", encoding="UTF-8") as f: | ||
f.write(app_log.log) | ||
spinner.stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor - probably want to wrap this in a try/catch block with the spinner.stop() in a finally statement in case writing to disk fails for any reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment in cluster submit below.
print(app_logs.log) | ||
app_log = spark_client.get_job_application_log(args.job_id, args.app_name) | ||
if args.output: | ||
spinner = utils.Spinner() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 26 through 30 seem to be used in several places. Worth moving to utils and re-using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just a few nitpicks and refactoring questions.
Fix #322