[SPARK-7977] [Build] Disallowing println

[jonalter]

No description provided.

[srowen]

It seems like a number of cases like this might be bad uses of println? I don't immediately see why this isn't a normal log

[jonalter]

Any guidelines on what should be a println or a log?
I'll be glad to go through and check them.

[tdas]

Is there any way to exclude the example files from this rule? The examples seems to require the ugly //scalastyle:off but println is totally okay in the examples.

[jonalter]

Checking out options for excluding examples

[jonalter]

@tdas scalastyle currently does not provide a way to exclude a folder (or files) from this rule. Wrapping the full example in //scalastyle:off may make it less intrusive than wrapping the specific printlns. Thoughts?

[tdas]

I think that is a better idea. Much less intrusive. Thank you for finding
this out :)

On Tue, Jun 30, 2015 at 9:43 AM, Jonathan Alter [email protected]
wrote:

@tdas https://github.com/tdas scalastyle currently does not provide a
way to exclude a folder (or files) from this rule. Wrapping the full
example in //scalastyle:off may make it less intrusive than wrapping the
specific printlns. Thoughts?

—
Reply to this email directly or view it on GitHub
#7093 (comment).

[jonalter]

@srowen - I have addressed the point you made regarding using log rather than println in some cases. Please let me know what you think. Thank you.

[srowen]

This is a big change. I still think a number of these printlns are debug leftovers and can be removed, or in some cases turned into logging. I don't know of a good way to review these other than to just sift through this a few times. I think anything in a main() method or close support of a CLI utility can stay; examples too are probably OK with println. Obviously there are some methods whose job it is to print to the console directly. Everything else, not sure where you would generally allow println.

So, this is one I think can be removed?

[srowen]

Heroic effort here. It certainly is big but is fixing up a lot of little issues of this form. I flagged a few more questions about it but it is looking pretty good.

Is anyone else concerned about the cost of future merge conflicts on this one vs the benefit?

[srowen]

Pinging people like @pwendell @JoshRosen @rxin @vanzin for thoughts about the scope of the change vs value -- anybody object?

[rxin]

+1 on the change.

Historically I don't think it doesn't make it much harder to backport fixes from small style changes. And most likely we don't backport printlns anyway.

The import one is slightly larger -- but even that I'm for merging it in towards the end of a release cycle.

[SparkQA]

Test build #1005 timed out for PR 7093 at commit 0b1dcb4 after a configured wait of 175m.

[SparkQA]

Test build #1010 timed out for PR 7093 at commit 10724b6 after a configured wait of 175m.

[srowen]

I can't figure out why this keeps timing out as it still doesn't look related to this change. Will try again later.

[jonalter]

I'm thinking the issue may actually be a failure and not just a timeout. Looks like it is having a serialization issue around ThreadingSuite.scala:126 which is right were I made a change. Checking it out.

[jonalter]

@srowen I confirmed the test is having trouble with logInfo here and works fine with println. Should I just revert this?

[srowen]

Oh darn, maybe the logging ends up dragging in some reference to the outer class. Maybe just zap this whole clause, or make the error message a field in this ThreadingSuiteState and use it below.

[jonalter]

Ooops, sorry about that.
Looks like we may not even need to save the message since the fail state and count is already being saved by ThreadingSuiteState.

[SparkQA]

Test build #1037 has finished for PR 7093 at commit ccd44cc.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • implicit class DebugQuery(query: DataFrame) extends Logging
  • class HiveContext(sc: SparkContext) extends SQLContext(sc) with Logging