[SPARK-8498][TUNGSTEN] fix npe in errorhandling path in unsafeshuffle writer

[holdenk]

No description provided.

[JoshRosen]

Spelling: I think this should be "propagates".

[JoshRosen]

Thanks for fixing this; the test looks good to me, but I think we might want to preserve the old behavior of also performing error cleanup when Throwables are thrown (i.e. by using the success flag idiom that I used before, but perhaps with a comment this time to explain what's going on).

[holdenk]

Sounds like a good plan.

[holdenk]

Would it not make more sense to just catch/rethrow throwables? (That way if fail during cleanup we can pass back an exception indicating that cleanup failed + the underlying cause as well)?

[JoshRosen]

I think that my concern was that wrapping a Throwable with a cause and re-throwing it might not be safe in general because catchers higher up the stack would then receive a different exception than the original Throwable, which might lead to problems if the throwable was an Error that's treated specially (e.g. ThreadDeath).

I suppose one approach would be to not re-wrap the Throwable when re-throwing. but AFAIK the compiler will complain if you re-throw Throwable in a method that doesn't declare that it throws Throwable. I think there's a reference for this in Guava's Throwables documentation: https://code.google.com/p/guava-libraries/wiki/ThrowablesExplained. It looks like this is a Java 6-ism, though, since Java 7 seems to allow this type of re-throwing: https://stackoverflow.com/questions/7428966/how-does-the-correct-rethrow-code-look-like-for-this-example

It looks like Java 7 provides us with a nice way to handle this with try-with-resources (see https://stuartmarks.wordpress.com/2011/07/21/a-new-java-exception-handling-idiom/), but I don't think we can use this here as long as we want to backport this fix to 1.4.1, which still must build for Java 6.

I think that's why I originally decided to go with the finally block, although I guess I should have added more try blocks to guard against errors during the cleanup itself.

[SparkQA]

Test build #35360 has finished for PR 6918 at commit e503b8c.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #35375 has finished for PR 6918 at commit 30e558d.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #35376 has finished for PR 6918 at commit 039d620.

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

[stephenh]

You could potentially use Utils.tryWithSafeFinally, which does this sort of annoying "try/catch in a try/catch" thing.

[JoshRosen]

This class, along with a lot of the other Tungsten code, is written in Java, so we can't use Utils.tryWithSafeFinally here.

[SparkQA]

Test build #35422 has finished for PR 6918 at commit 855f9aa.

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

[JoshRosen]

Shouldn't we log in the else branch to make sure that the error is never silently swallowed?

[JoshRosen]

LGTM pending Jenkins; thanks!

[SparkQA]

Test build #35528 has finished for PR 6918 at commit f807832.

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

[JoshRosen]

Merging this to master and branch-1.4.

[JoshRosen]

Dammit, I just realized that the catch Exception added in the latest commit will break compilation under Java 6 in branch-1.4 :(. I'm going to revert in branch-1.4 and will just push a hotfix later to address this (using either Guava or Unsafe to handle re-throw).