Skip to content

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Oct 28, 2016

What changes were proposed in this pull request?

Fixed the issue that ForeachSink didn't rethrow the exception.

How was this patch tested?

The fixed unit test.

@zsxwing
Copy link
Member Author

zsxwing commented Oct 28, 2016

cc @tdas

@SparkQA
Copy link

SparkQA commented Oct 28, 2016

Test build #67715 has finished for PR 15674 at commit 4466010.

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

writer.close(e)
throw e
}
if (!isFailed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a better of write this now using try...catch...finally. Because the condition here will always be true

writer.process(iter.next())
}
} catch {
case e: Throwable =>
Copy link
Contributor

@tdas tdas Oct 28, 2016

Choose a reason for hiding this comment

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

wait.. isnt it simpler to do

try {
  ...
} catch {
 case e => 
    logError(.....)
    throw e
} finally {
   writer.close(e)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

cannot access e inside finally.

Copy link
Contributor

Choose a reason for hiding this comment

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

oops. my bad.

@tdas
Copy link
Contributor

tdas commented Oct 28, 2016

LGTM pending tests. Should merge to master and 2.0

@SparkQA
Copy link

SparkQA commented Oct 29, 2016

Test build #67730 has finished for PR 15674 at commit a214755.

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

@zsxwing
Copy link
Member Author

zsxwing commented Oct 29, 2016

Merging to master and 2.0. Thanks, @tdas

asfgit pushed a commit that referenced this pull request Oct 29, 2016
… throws exception

## What changes were proposed in this pull request?

Fixed the issue that ForeachSink didn't rethrow the exception.

## How was this patch tested?

The fixed unit test.

Author: Shixiong Zhu <[email protected]>

Closes #15674 from zsxwing/foreach-sink-error.

(cherry picked from commit 59cccbd)
Signed-off-by: Shixiong Zhu <[email protected]>
@asfgit asfgit closed this in 59cccbd Oct 29, 2016
@zsxwing zsxwing deleted the foreach-sink-error branch October 29, 2016 03:17
robert3005 pushed a commit to palantir/spark that referenced this pull request Nov 1, 2016
… throws exception

## What changes were proposed in this pull request?

Fixed the issue that ForeachSink didn't rethrow the exception.

## How was this patch tested?

The fixed unit test.

Author: Shixiong Zhu <[email protected]>

Closes apache#15674 from zsxwing/foreach-sink-error.
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
… throws exception

## What changes were proposed in this pull request?

Fixed the issue that ForeachSink didn't rethrow the exception.

## How was this patch tested?

The fixed unit test.

Author: Shixiong Zhu <[email protected]>

Closes apache#15674 from zsxwing/foreach-sink-error.
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