Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updates to spark job type #205

Merged
merged 11 commits into from
Oct 20, 2015
Merged

updates to spark job type #205

merged 11 commits into from
Oct 20, 2015

Conversation

johnyu0520
Copy link
Contributor

for Spark: removing the code for setting default values …
addressed #204 comments
bug fix in SparkJobArgs.java
make build completion dependent on passing plugins/jobtype junit test cases

This is basically the same code reviewed in #204, with a few recent commits squashed (I won't touch the commits from 3 weeks ago as it would be a lot of conflicts)

https://github.com/azkaban/azkaban-plugins/pull/204/files

@davidzchen
Copy link
Contributor

The diff itself looks good.

There are still multiple commits here. Can you squash them into a single commit? Feel free to let me know if you need any help doing the squash.

@johnyu0520
Copy link
Contributor Author

Hey David,

many of those commits are over 2 weeks ago and intertwined with other already merged into master changes. I would rather not touch them...

@davidzchen
Copy link
Contributor

Understood. That is fine for this PR then. Thanks for taking the time to squash the other commits!

@@ -3,61 +3,54 @@
public enum SparkJobArg {

// standard spark submit arguments, ordered in the spark-submit --help order
MASTER("master", "yarn-cluster", false), // just to trick the eclipse formatter
MASTER("master", false), // just to trick the eclipse formatter
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, what is the motivation for removing the default values?

@davidzchen
Copy link
Contributor

LGTM

@johnyu0520
Copy link
Contributor Author

@davidzchen
Thanks for the review. I'll definitely be more careful about commit management in the future!

@davidzchen
Copy link
Contributor

No problem. Thanks, @johnyu0520!

@logiclord
Copy link
Contributor

LGTM

logiclord added a commit that referenced this pull request Oct 20, 2015
@logiclord logiclord merged commit fc3651f into azkaban:master Oct 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants