-
Notifications
You must be signed in to change notification settings - Fork 394
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
Make FileOutputCommiter as default #86
Make FileOutputCommiter as default #86
Conversation
Codecov Report
@@ Coverage Diff @@
## master #86 +/- ##
=========================================
Coverage ? 86.07%
=========================================
Files ? 292
Lines ? 9521
Branches ? 548
=========================================
Hits ? 8195
Misses ? 1326
Partials ? 0
Continue to review full report at Codecov.
|
docs/installation/index.md
Outdated
@@ -5,3 +5,13 @@ | |||
* Clone the TransmogrifAI repo: `git clone https://github.com/salesforce/TransmogrifAI.git` | |||
* Build the project: `cd TransmogrifAI && ./gradlew compileTestScala installDist` | |||
* Start hacking | |||
|
|||
# (Optional) Configuration |
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.
Not aure if this belongs here or we need a new section. @snabar @leahmcguire wdyt?
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.
I think we don't need to document output committer conf prominently since this is well documented for Hadoop and by extension for Spark itself. So probably not the time to modify index.md at this time either.
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.
Please grep the whole codebase for the removed class names and make aure you remove them all. I think build.gradle has some reference to them in license plugin config for instance.
@tovbinm ran a case-insensitive grep for both classes. Didn't find any new occurrences. |
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.
LGTM, but let us not duplicate hadoop/spark documentation unless on a wiki or FAQ if it comes up a lot.
@ajayborra have you tried running the helloworld after the change? Otherwise lgtm |
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.
Please test with helloword, otherwise lgtm
@tovbinm Yes ran iris dataset example by setting output destination to s3. Status is green. |
Related issues
#54
Changes