[SPARK-4379][Core] Change Exception to SparkException in checkpoint

[zsxwing]

It's better to change to SparkException. However, it's a breaking change since it will change the exception type.

[SparkQA]

Test build #23304 has started for PR 3241 at commit 409f3af.

• This patch merges cleanly.

[srowen]

Yes, throwing Exception or RuntimeException is bad practice in Java. That said, I do think you're right that it changes the signature in the bytecode, since this is a checked exception. I don't think it's worth that. At the least, rather than change this once, change it everywhere and wait until a breaking change is acceptable.

[SparkQA]

Test build #23304 has finished for PR 3241 at commit 409f3af.

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

[AmplabJenkins]

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

[pwendell]

Does this definitely change the bytecode signature? If so, I wonder why we don't get a compatiblity warning.

[JoshRosen]

We're throwing a more specific exception class, so I guess that caller code doesn't need to change: if the old caller code caught Exception then it can certainly catch SparkException since that's a subclass. Things would be different if we changed this to throw a wider exception type.

I think this compatibility concern is moot, though, since checkpoint() wasn't declared with the @throws Scala annotation, so the generated code method won't have any declared exceptions:

scala> import java.io.IOException
import java.io.IOException

scala> class A {
     |   def foo() {
     |    throw new IOException("error!")
     |   }
     | }
defined class A

scala> :javap -s A
Compiled from "<console>"
public class A {
  public void foo();
    Signature: ()V

  public A();
    Signature: ()V
}

scala> class B {
     |   @throws[IOException]
     |   def foo() {
     |      throw new IOException("error!")
     |   }
     | }
defined class B

scala> :javap -s B
Compiled from "<console>"
public class B {
  public void foo() throws java.io.IOException;
    Signature: ()V

  public B();
    Signature: ()V
}

[srowen]

Hm, I may have learned something today. I don't think it is even in the signature of the method when scalac compiles it. See http://www.scala-lang.org/old/node/106 or http://engineering.richrelevance.com/exceptional-exceptions/#advanced Sure enough javap says that the exception attributes aren't in the bytecode. So I don't think this does change the signature after all.

[andrewor14]

Does that mean if we went the other way (i.e. changing it from SparkException to Exception) it would be bytecode incompatible?

[zsxwing]

Exception won't be in the method signature.

scala> import java.io.IOException
import java.io.IOException

scala> class A {
     |   @throws[IOException]
     |   def foo() {
     |     throw new IOException("error!")
     |   }
     | 
     |   def bar(): Unit = {
     |     foo()
     |   }
     | }
defined class A

scala> :javap -private -c A
Compiled from "<console>"
public class A extends java.lang.Object{
public void foo()   throws java.io.IOException;
  Code:
   0:   new #9; //class java/io/IOException
   3:   dup
   4:   ldc #11; //String error!
   6:   invokespecial   #15; //Method java/io/IOException."<init>":(Ljava/lang/String;)V
   9:   athrow

public void bar();
  Code:
   0:   aload_0
   1:   invokevirtual   #20; //Method foo:()V
   4:   return

public A();
  Code:
   0:   aload_0
   1:   invokespecial   #22; //Method java/lang/Object."<init>":()V
   4:   return

}

In the bar(), the instruction is 1: invokevirtual #20; //Method foo:()V. The method is still foo:()V.

[zsxwing]

I'm sorry that I should have been clearer when I said the breaking change. I'm worried about the following case:

  try {
    rdd.checkpoint()
  } catch {
    case e: SparkException => // do work A
    case e: Exception => do work B
  }

It breaks such case. However, I think few people will write such code.

Therefore, does Spark view such change as a breaking change?

[rxin]

I think this should be ok. Because we didn't really document the exception, it is not strictly part of the contract.

[rxin]

I'm merging this in master.