[SPARK-11615] Drop @VisibleForTesting annotation

[tedyu]

See http://search-hadoop.com/m/q3RTtjpe8r1iRbTj2 for discussion.

Summary: addition of @VisibleForTesting annotation resulted in spark-shell malfunctioning.

[tedyu]

May need a bit hint on how style-checker rule should be composed to prevent regression

[tedyu]

Here is listing of files where @VisibleForTesting is used:

import com.google.common.annotations.VisibleForTesting
  @VisibleForTesting
./core/src/main/scala/org/apache/spark/rpc/netty/Inbox.scala
import com.google.common.annotations.VisibleForTesting
./core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala
import com.google.common.annotations.VisibleForTesting
  @VisibleForTesting
  @VisibleForTesting
./core/src/main/scala/org/apache/spark/util/AsynchronousListenerBus.scala
import com.google.common.annotations.VisibleForTesting
  @VisibleForTesting
./core/src/main/scala/org/apache/spark/util/collection/ExternalSorter.scala
import com.google.common.annotations.VisibleForTesting
  @VisibleForTesting
./sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala
import com.google.common.annotations.VisibleForTesting
./yarn/src/test/scala/org/apache/spark/network/shuffle/ShuffleTestAccessor.scala

Do we want to remove all occurrences ?

[marmbrus]

Can you create a JIRA please.

@JoshRosen for hints on the style checker.

[tedyu]

I can follow classforname rule from scalastyle-config.xml - if we decide that @VisibleForTesting should not be used at all

[SparkQA]

Test build #45481 has finished for PR 9585 at commit 3021037.

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

[JoshRosen]

What I'd probably do is just ban the import via a string pattern match checker, then replace the existing occurrences with comments or introduce a Spark-specific version of the annotation and use that instead.

[SparkQA]

Test build #45488 has finished for PR 9585 at commit 3548fb8.

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

[tedyu]

Added a rule in scalastyle-config.xml

[SparkQA]

Test build #45526 has finished for PR 9585 at commit 62f3bc7.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[tedyu]

@JoshRosen
Can you take a look ?

[andrewor14]

It's weird to have // VisibleForTesting as a comment. I'd say we just put it in the java doc of the method, e.g.

/**
 * This method does something...
 * Exposed for testing.
 */

For the custom message here I'd just say the annotation is banned or something.

[SparkQA]

Test build #45529 has finished for PR 9585 at commit 81a0462.

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

[tedyu]

From https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/45529/consoleFull :

DONE 
Tests passed.

[tedyu]

From https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/45532/console :
{code}
Caused by: hudson.plugins.git.GitException: Command "git fetch --tags --progress https://github.com/apache/spark.git +refs/pull/9585/:refs/remotes/origin/pr/9585/" returned status code 143:
stdout:
stderr: error: RPC failed; result=18, HTTP code = 200
{code}

[vanzin]

super nit: indentation is weird.

[vanzin]

I kinda like that annotation, much more than a comment in the scaladoc, but well... LGTM. retest this please

[andrewor14]

should be java doc, not scala doc

[SparkQA]

Test build #45557 has finished for PR 9585 at commit d5da5ea.

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

[tedyu]

[info] *** 1 TEST FAILED ***
[error] Failed: Total 19, Failed 1, Errors 0, Passed 18, Ignored 1
[error] Failed tests:
[error]     org.apache.spark.sql.hive.thriftserver.CliSuite

Not related to this PR

[SparkQA]

Test build #45554 has finished for PR 9585 at commit dacec5e.

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

[SparkQA]

Test build #45561 has finished for PR 9585 at commit 29fe9e4.

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