Skip to content
This repository has been archived by the owner on Feb 3, 2021. It is now read-only.

Feature: Spark add output logs flag #468

Merged
merged 6 commits into from
Apr 5, 2018

Conversation

jafreck
Copy link
Member

@jafreck jafreck commented Mar 28, 2018

Fix #322

@jafreck jafreck modified the milestones: v0.8.0, v0.7.0 Mar 28, 2018
@@ -1,3 +1,4 @@
import os
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: alpha sort

Copy link
Contributor

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()
Copy link
Contributor

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.

Copy link
Contributor

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()
Copy link
Contributor

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?

Copy link
Contributor

@paselem paselem left a 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.

@jafreck jafreck merged commit 32de752 into Azure:master Apr 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants