Skip to content

Conversation

@tedyu
Copy link
Contributor

@tedyu tedyu commented Feb 26, 2015

...ns#saveAsNewAPIHadoopDataset

Copy link
Member

Choose a reason for hiding this comment

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

Why check here? if it's null then this fails already in the loop. Just assert it's not null with require after it's obtained, if this is supposed to die in this case anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, updated as suggested.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

Copy link
Member

Choose a reason for hiding this comment

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

There's no string interpolation here, and there is no actual NewRecordWriter class (it's an alias). Don't worry I can edit that before merge. I will have Jenkins test just for completeness

@srowen
Copy link
Member

srowen commented Feb 26, 2015

OK to test

@srowen
Copy link
Member

srowen commented Feb 26, 2015

Jenkins, test this please

@SparkQA
Copy link

SparkQA commented Feb 26, 2015

Test build #28021 has started for PR 4794 at commit 2632a57.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 26, 2015

Test build #28021 has finished for PR 4794 at commit 2632a57.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28021/
Test PASSed.

@srowen
Copy link
Member

srowen commented Feb 26, 2015

OK this looks like no-risk small fix. I'll adjust it and merge.

@asfgit asfgit closed this in e60ad2f Feb 26, 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.

4 participants