[SPARK-10227] fatal warnings with sbt on Scala 2.11

[skyluc]

The bulk of the changes are on @transient annotation on class parameter. Often the compiler doesn't generate a field for this parameters, so the the transient annotation would be unnecessary.
But if the class parameter are used in methods, then fields are created. So it is safer to keep the annotations.

The remainder are some potential bugs, and deprecated syntax.

[srowen]

Hm, tough call. If the code is acting like this is a field, and it's not, then that seems like a problem to solved in itself. I think a lot of the annotations could be removed. For example, initialValue here should never become a field in this object.

I get the potential problem, that code changes in a way that makes this a field. If the field isn't serializable, it starts to fail. Unit tests would (hopefully) catch it. But, I also think that having such a new field become transient isn't a good solution; it piles an unintended change on an unintended change.

In fact, I think a lot of current usages of @transient in the code are poor practice anyway; they exist to make an object serialize, but it's not clear that the object makes sense without the field after deserialization.

What do you / others think about just removing these unneeded annotations?

[skyluc]

My original fix was to remove the unneeded annotations, and to transform the parameter where the annotation was making sense into private val. But it looks so much like a common pattern, that I didn't change it.

[dragos]

/cc @rxin since we talked about this before, and he's the one who added -XfatalWarnings to the build.

[rxin]

What's the meaning of param and field?

The scala documentation doesn't really say anything: http://www.scala-lang.org/api/2.10.4/index.html#scala.annotation.meta.param

"Consult the documentation in package scala.annotation.meta."

But scala.annotation.meta doesn't say much either.

[skyluc]

It indicates on which of the elements generated for the class parameter to apply the annotation.
There are 4 possible elements generated for a class parameter: the constructor parameter, the field, the getter and the setter (and more for beans).
I would expect @(transient @field) to be enough, for the cases when a field is actually created, but the warning is not resolve until @param is added too. I haven't dig on why the compiler complain in this case.

The base question was mostly: do we want to remove the @transient annotation which have no effect at all (no field generated by the compiler).

[srowen]

@rxin et al I think I'd favor removing these @transient. They don't do anything now. The only argument for the more complex expansion is to quietly apply transient if the param quietly becomes a field, and that does not seem like a good automatic thing to happen. @skyluc I would favor moving forward with that variation unless anyone objects shortly.

[skyluc]

Updated the changes for @transient. The annotation has been removed where not needed, and the paremeter has been transformed in @transient private val where needed.

[SparkQA]

Test build #1711 has finished for PR 8433 at commit 0408404.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SparkHadoopWriter(jobConf: JobConf)
  • class BlockRDD[T: ClassTag](sc: SparkContext, @transient val blockIds: Array[BlockId])
  • class ZippedWithIndexRDD[T: ClassTag](prev: RDD[T]) extends RDD[(T, Long)](prev)
  • class Checkpoint(ssc: StreamingContext, val checkpointTime: Time)
  • abstract class InputDStream[T: ClassTag] (ssc_ : StreamingContext)
  • abstract class ReceiverInputDStream[T: ClassTag](ssc_ : StreamingContext)

[dragos]

Is this import still needed?

[skyluc]

No, it is a left over of the previous changes.

[SparkQA]

Test build #1719 has finished for PR 8433 at commit 0408404.

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

[srowen]

Aside from possible removing those imports, yes, this looks good to me for master/1.6.

[srowen]

Just giving this one more go through Jenkins, but still LGTM. our test framework is just testing Scala 2.10, not 2.11. However, it seems like this patch can only make it less broken for Scala 2.11 + SBT if it doesn't work now, so I think the risk of merging into master is low. The changes seem sound and positive in any event.

[SparkQA]

Test build #1726 has finished for PR 8433 at commit 5892876.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SparkHadoopWriter(jobConf: JobConf)
  • class BlockRDD[T: ClassTag](sc: SparkContext, @transient val blockIds: Array[BlockId])
  • class ZippedWithIndexRDD[T: ClassTag](prev: RDD[T]) extends RDD[(T, Long)](prev)
  • class Checkpoint(ssc: StreamingContext, val checkpointTime: Time)
  • abstract class InputDStream[T: ClassTag] (ssc_ : StreamingContext)
  • abstract class ReceiverInputDStream[T: ClassTag](ssc_ : StreamingContext)