[SPARK-23550][core] Cleanup `Utils`. #20706

vanzin · 2018-03-01T19:14:02Z

A few different things going on:

Remove unused methods.
Move JSON methods to the only class that uses them.
Move test-only methods to TestUtils.
Make getMaxResultSize() a config constant.
Reuse functionality from existing libraries (JRE or JavaUtils) where possible.

The change also includes changes to a few tests to call Utils.createTempFile correctly,
so that temp dirs are created under the designated top-level temp dir instead of
potentially polluting git index.

A few different things going on: - Remove unused methods. - Move JSON methods to the only class that uses them. - Move test-only methods to TestUtils. - Make getMaxResultSize() a config constant. - Reuse functionality from existing libraries (JRE or JavaUtils) where possible. There is a slight change in the behavior of "createDirectory()": it does not create a full directory tree anymore, just the directory being requested. That's better since the code would only set up proper permissions for the leaf directory, and this change requires its parent to already exist.

rxin · 2018-03-01T19:26:04Z

core/src/main/scala/org/apache/spark/util/Utils.scala

-          dir = null
-        }
-      } catch { case e: SecurityException => dir = null; }
+    val prefix = namePrefix + "-"


was there a reason you rewriting this?

This code was written when Spark still supported Java 1.6, and java.nio.file did not exist.

hvanhovell · 2018-03-01T19:51:47Z

core/src/main/scala/org/apache/spark/util/Utils.scala

+    val prefix = namePrefix + "-"
+    val dir = if (root != null) {
+      val rootDir = new File(root)
+      require(rootDir.isDirectory(), s"Root path $root must be an existing directory.")


Why not use the nio api's? e.g.:

val rootDir = Paths.get(root) require(Files.isDirectory(rootDir), s"Root path $root must be an existing directory.") Files.createTempDirectory(rootDir, prefix)

Same thing in the end, no? This method needs to return File regardless of what it does internally.

That is also an argument for not changing this method at all. Just use File.createTempFile(...) and be done with it.

This is for a directory, not file. File.createTempFile does not work.

Yeah, I missed that sorry. However my point still stands.

Not sure what that point really is, but if you feel so strongly about using the nio APIs, that's easy enough.

gatorsmile · 2018-03-01T20:00:49Z

The behavior changes have to be separated from the code refactoring.

vanzin · 2018-03-01T20:16:00Z

The behavior changes have to be separated from the code refactoring.

The behavior change only affects tests, which have been fixed in this PR.

gatorsmile · 2018-03-01T20:50:49Z

Unfortunately, I do not know whether this change only affects the test cases. Any behavior change require a conf. We can get rid of it later if nobody hit the issue.

vanzin · 2018-03-01T20:59:21Z

I do not know whether this change only affects the test cases.

I do, because I checked. The tests were using the API incorrectly, actually, and could leave garbage outside the temp dirs, polluting the source tree. That wrong usage pattern was actually the thing that made tests be affected by the change. No code outside of tests uses the API in that way.

And adding a conf would kinda defeat the purpose of using the existing JRE API, since it would require maintaining the old code.

In summary, not all behavior changes are equal, especially if you're talking about an internal API.

That being said, I'm going to run some tests with YARN+Kerneros+ShuffleService to make sure things are the same - I think I may need to adjust something in DiskBlockManager.

vanzin · 2018-03-01T21:26:39Z

Ok, I'm going to revert the createDirectory changes because the Files.createTempDirectory api forces permissions to 700, and secure YARN requires that the block manager directories respect the umask set for the container or things break. I could fix that in DiskBlockManager but reverting sounds safer.

I'll keep the test changes since they're correct and fix a potential issue with garbage being left around the working dir.

SparkQA · 2018-03-01T22:47:43Z

Test build #87850 has finished for PR 20706 at commit 4b8b8e2.

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

SparkQA · 2018-03-02T00:00:35Z

Test build #87853 has finished for PR 20706 at commit c239d96.

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

SparkQA · 2018-03-02T01:16:04Z

Test build #87855 has finished for PR 20706 at commit d026fff.

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

vanzin · 2018-03-02T01:20:30Z

I'm almost sure there's a bug somewhere for that flaky test, but if there isn't I'll file one.

vanzin · 2018-03-02T01:20:48Z

retest this please

SparkQA · 2018-03-02T05:36:27Z

Test build #87863 has finished for PR 20706 at commit d026fff.

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

jiangxb1987

I looked at some of the changes that move methods out of Utils, they seems valid till now, but I'm not sure whether these are good move because I'm afraid we may have to use them somewhere else in the future and then we have to move them back to Utils. I really feel we shall be conservative to make these changes.

jiangxb1987 · 2018-03-02T15:37:45Z

core/src/main/scala/org/apache/spark/util/Utils.scala

-  /**
-   * Lists files recursively.
-   */
-  def recursiveList(f: File): Array[File] = {


Yes this is only used by test suites for now, but can you still keep this in Utils so we don't have to move it back when we want to use it in the production code (I assume this may happen someday).

I disagree that we should keep things in production code "just in case".

This is also an example of a function that if used without care is dangerous. Files.walk is a much better way of achieving this.

jiangxb1987 · 2018-03-02T15:43:40Z

core/src/main/scala/org/apache/spark/util/Utils.scala

@@ -1876,17 +1822,6 @@ private[spark] object Utils extends Logging {
    obj.getClass.getSimpleName.replace("$", "")
  }

-  /** Return an option that translates JNothing to None */
-  def jsonOption(json: JValue): Option[JValue] = {


This is only used in JSONProtocol for now, but this is also a useful JSON method that could apply to some other JSON operations in the future.

We should not be using json4s for new things, so it's better to have these hidden away.

What is the reason we should not use json4s for new things?

It encourages writing manual serialization and deserialization code, which is error prone.

hvanhovell · 2018-03-03T12:17:59Z

@jiangxb1987 keeping dead code around will inevitably lead to code rot. If there is not use for something then we should remove it. Adding things back is not a problem.

SparkQA · 2018-03-06T04:02:14Z

Test build #87984 has finished for PR 20706 at commit 427a977.

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

vanzin · 2018-03-07T19:35:56Z

Any more comments?

gatorsmile · 2018-03-07T21:40:34Z

LGTM

Thanks! Merged to master.

rxin reviewed Mar 1, 2018

View reviewed changes

hvanhovell reviewed Mar 1, 2018

View reviewed changes

More nio.

c239d96

Revert to previous createDirectory(), which follows umask.

d026fff

jiangxb1987 reviewed Mar 2, 2018

View reviewed changes

Merge branch 'master' into SPARK-23550

427a977

asfgit closed this in c99fc9a Mar 7, 2018

vanzin deleted the SPARK-23550 branch March 12, 2018 20:28

[SPARK-23550][core] Cleanup Utils. #20706

[SPARK-23550][core] Cleanup Utils. #20706

Uh oh!

Conversation

vanzin commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Mar 1, 2018

Uh oh!

vanzin commented Mar 1, 2018

Uh oh!

gatorsmile commented Mar 1, 2018

Uh oh!

vanzin commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanzin commented Mar 1, 2018

Uh oh!

SparkQA commented Mar 1, 2018

Uh oh!

SparkQA commented Mar 2, 2018

Uh oh!

SparkQA commented Mar 2, 2018

Uh oh!

vanzin commented Mar 2, 2018

Uh oh!

vanzin commented Mar 2, 2018

Uh oh!

SparkQA commented Mar 2, 2018

Uh oh!

jiangxb1987 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented Mar 3, 2018

Uh oh!

SparkQA commented Mar 6, 2018

Uh oh!

vanzin commented Mar 7, 2018

Uh oh!

gatorsmile commented Mar 7, 2018

Uh oh!

Uh oh!

[SPARK-23550][core] Cleanup `Utils`. #20706

[SPARK-23550][core] Cleanup `Utils`. #20706

vanzin commented Mar 1, 2018 •

edited

Loading

vanzin commented Mar 1, 2018 •

edited

Loading

jiangxb1987 left a comment •

edited

Loading