[SPARK-1779] Throw an exception if memory fractions are not between 0 and 1 #714

scwf · 2014-05-09T12:14:40Z

No description provided.

add a warning when memoryFraction is not between 0 and 1

AmplabJenkins · 2014-05-09T12:17:58Z

Can one of the admins verify this patch?

andrewor14 · 2014-05-09T16:01:20Z

We should probably throw an exception, actually. It's a pathological state if the max memory is negative, or greater than what the JVM offers.

andrewor14 · 2014-05-09T16:02:22Z

Could you also do that in ExternalAppendOnlyMap? There are two similar configs there.

scwf · 2014-05-09T16:53:57Z

ok, updated.

andrewor14 · 2014-05-09T16:56:50Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

@@ -1045,6 +1045,9 @@ private[spark] object BlockManager extends Logging {

  def getMaxMemory(conf: SparkConf): Long = {
    val memoryFraction = conf.getDouble("spark.storage.memoryFraction", 0.6)
+    if (memoryFraction > 1 && memoryFraction <= 0) {


nit: the value of 0 is technically "between 0 and 1." This should be memoryFraction < 0. Someone might want to not use the RDD cache at all and use all memory for spilling, for instance. (though I don't think this is common)

(Same in ExternalAppendOnlyMap)

andrewor14 · 2014-05-10T02:11:54Z

Thanks, this looks good. (now we just need to test it)

witgo · 2014-05-10T02:34:31Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

@@ -1045,6 +1045,9 @@ private[spark] object BlockManager extends Logging {

  def getMaxMemory(conf: SparkConf): Long = {
    val memoryFraction = conf.getDouble("spark.storage.memoryFraction", 0.6)
+    if (memoryFraction > 1 && memoryFraction < 0) {


Should throw an IllegalArgumentException.

Preconditions.checkArgument(0 < memoryFraction && 1 > memoryFraction, "spark.storage.memoryFraction should be between 0 and 1.")

thanks for that

witgo · 2014-05-10T02:36:29Z

【SPARK-1779】 => [SPARK-1779]

watermen · 2014-05-10T07:28:45Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

@@ -76,6 +76,12 @@ class ExternalAppendOnlyMap[K, V, C](
  private val maxMemoryThreshold = {
    val memoryFraction = sparkConf.getDouble("spark.shuffle.memoryFraction", 0.3)
    val safetyFraction = sparkConf.getDouble("spark.shuffle.safetyFraction", 0.8)
+    if (memoryFraction > 1 && memoryFraction < 0) {


Should or(||) instead of and(&&).
[memoryFraction > 1 && memoryFraction < 0] ＝> [memoryFraction > 1 || memoryFraction < 0]
The same to safetyFraction.

yeah, i made a mistake

oops, good call

pwendell · 2014-05-10T17:19:46Z

Hey @witgo @andrewor14. I thought I commented earlier but it appears the comment didn't get posted.

I think it would be a good idea to validate this at the time of SparkConf construction rather than have validations scattered throughout the code.

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/SparkConf.scala#L213

Also - it would be good to echo back the value that was set to the user. Sometimes users don't know where a configuration option is coming from:

if (memoryFraction > 1 || memoryFraction < 0) {
  val msg = s"spark.storage.memoryFraction should be between 0 and 1 (was '$memoryFraction')."
  throw new IllegalArgumentException(msg)
}

scwf · 2014-05-11T13:09:36Z

Hi @pwendell, thanks. I didn't notice validateSettings in SparkConf and it's good to validate this in it.

erikerlandson · 2014-05-13T17:10:53Z

I coded up a proposal where SparkConf owns the checking inside of getInt(), getDouble() and friends, described here:

https://issues.apache.org/jira/browse/SPARK-1779?focusedCommentId=13996567&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13996567

pwendell · 2014-05-28T05:59:10Z

core/src/main/scala/org/apache/spark/SparkConf.scala

@@ -236,6 +236,27 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
      }
    }

+    // Validate memoryFraction
+    val storageMemFraction = getDouble("spark.storage.memoryFraction", 0.6)


This could be written a bit more concisely like:

val memoryKeys = Seq( "spark.storage.memoryFraction", "spark.shuffle.memoryFraction", "spark.shuffle.safetyFraction") for (key -> memoryKeys) { val value = getDouble(key, 0.5) if (value > 1 || value < 0) { throw new IllegalArgumentException("$key should be between 0 and (was '$value').") } }

ok, updated!

pwendell · 2014-05-28T05:59:37Z

Suggested a way to make this more concise, but looks good overall.

andrewor14 · 2014-06-16T16:48:52Z

core/src/main/scala/org/apache/spark/SparkConf.scala

+    for (key -> memoryKeys) {
+      val value = getDouble(key, 0.5)
+      if (value > 1 || value < 0) {
+      throw new IllegalArgumentException("$key should be between 0 and (was '$value').")


missing indent

andrewor14 · 2014-06-16T16:49:44Z

Ah yes, this is much better. There is a missing indent, however. After you fix that I think this is good to go.

pwendell · 2014-06-21T01:39:09Z

Jenkins, test this please.

scwf · 2014-08-04T08:21:49Z

hey @andrewor14 ， i can not see FAILED unit tests info, so i do not know how to resolve it. can you help me

andrewor14 · 2014-08-04T18:54:45Z

test this please

andrewor14 · 2014-08-04T18:55:41Z

Pending tests this LGTM

SparkQA · 2014-08-04T18:59:34Z

QA tests have started for PR 714. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17865/consoleFull

SparkQA · 2014-08-04T19:00:22Z

QA results for PR 714:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17865/consoleFull

andrewor14 · 2014-08-04T20:08:27Z

core/src/main/scala/org/apache/spark/SparkConf.scala

@@ -236,6 +236,20 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
      }
    }

+    // Validate memoryFraction


nit: this is no longer only memoryFraction. Maybe use "Validate memory fractions" instead

andrewor14 · 2014-08-04T20:10:59Z

Also, could you update the description to "Throw an exception if memory fractions are not between 0 and 1"

scwf · 2014-08-05T01:36:32Z

@andrewor14 if so, we do not know which one is outside (0,1)

andrewor14 · 2014-08-05T02:01:46Z

What do you mean? I was referring to the title of the PR, because it's now out of date based on your recent changes.

andrewor14 · 2014-08-05T02:02:22Z

test this please

SparkQA · 2014-08-05T02:04:20Z

QA tests have started for PR 714. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17909/consoleFull

pwendell · 2014-08-05T02:08:50Z

LGTM pending tests.

SparkQA · 2014-08-05T02:59:30Z

QA results for PR 714:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17909/consoleFull

scwf · 2014-08-05T04:10:54Z

hi @andrewor14，unit tests error in sparkstreaming，we may retest this

[info] - flume polling test multiple hosts *** FAILED ***
[info] org.jboss.netty.channel.ChannelException: Failed to bind to: localhost/127.0.0.1:56218
[info] at org.jboss.netty.bootstrap.ServerBootstrap.bind(ServerBootstrap.java:272)

andrewor14 · 2014-08-05T05:48:09Z

test this please

pwendell · 2014-08-05T06:46:07Z

Jenkins, test this please.

SparkQA · 2014-08-05T06:49:44Z

QA tests have started for PR 714. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17932/consoleFull

SparkQA · 2014-08-05T07:50:16Z

QA results for PR 714:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17932/consoleFull

pwendell · 2014-08-05T07:50:54Z

Thanks - I'm going to merge this.

… and 1 Author: wangfei <[email protected]> Author: wangfei <[email protected]> Closes #714 from scwf/memoryFraction and squashes the following commits: 6e385b9 [wangfei] Update SparkConf.scala da6ee59 [wangfei] add configs 829a195 [wangfei] add indent 717c0ca [wangfei] updated to make more concise fc45476 [wangfei] validate memoryfraction in sparkconf 2e79b3d [wangfei] && => || 43621bd [wangfei] && => || cf38bcf [wangfei] throw IllegalArgumentException 14d18ac [wangfei] throw IllegalArgumentException dff1f0f [wangfei] Update BlockManager.scala 764965f [wangfei] Update ExternalAppendOnlyMap.scala a59d76b [wangfei] Throw exception when memoryFracton is out of range 7b899c2 [wangfei] 【SPARK-1779】

… and 1 Author: wangfei <[email protected]> Author: wangfei <[email protected]> Closes apache#714 from scwf/memoryFraction and squashes the following commits: 6e385b9 [wangfei] Update SparkConf.scala da6ee59 [wangfei] add configs 829a195 [wangfei] add indent 717c0ca [wangfei] updated to make more concise fc45476 [wangfei] validate memoryfraction in sparkconf 2e79b3d [wangfei] && => || 43621bd [wangfei] && => || cf38bcf [wangfei] throw IllegalArgumentException 14d18ac [wangfei] throw IllegalArgumentException dff1f0f [wangfei] Update BlockManager.scala 764965f [wangfei] Update ExternalAppendOnlyMap.scala a59d76b [wangfei] Throw exception when memoryFracton is out of range 7b899c2 [wangfei] 【SPARK-1779】

* drop user info when ingesting conda list explicit uris * add test * special taste of scala * more test * fix test * more docs * 1 more docs

【SPARK-1779】

7b899c2

add a warning when memoryFraction is not between 0 and 1

Throw exception when memoryFracton is out of range

a59d76b

andrewor14 reviewed May 9, 2014
View reviewed changes

wangfei added 2 commits May 10, 2014 09:03

Update ExternalAppendOnlyMap.scala

764965f

Update BlockManager.scala

dff1f0f

witgo reviewed May 10, 2014
View reviewed changes

scwf changed the title ~~【SPARK-1779】add warning when memoryFraction is not between 0 and 1~~ [SPARK-1779] add warning when memoryFraction is not between 0 and 1 May 10, 2014

wangfei added 2 commits May 10, 2014 11:21

throw IllegalArgumentException

14d18ac

throw IllegalArgumentException

cf38bcf

watermen reviewed May 10, 2014
View reviewed changes

wangfei added 2 commits May 10, 2014 16:18

&& => ||

43621bd

&& => ||

2e79b3d

validate memoryfraction in sparkconf

fc45476

pwendell reviewed May 28, 2014
View reviewed changes

updated to make more concise

717c0ca

andrewor14 reviewed Jun 16, 2014
View reviewed changes

add indent

829a195

andrewor14 reviewed Aug 4, 2014
View reviewed changes

Update SparkConf.scala

6e385b9

scwf changed the title ~~[SPARK-1779] add warning when memoryFraction is not between 0 and 1~~ [SPARK-1779] Throw an exception if memory fractions are not between 0 and 1 Aug 5, 2014

asfgit closed this in 9862c61 Aug 5, 2014

scwf deleted the memoryFraction branch August 22, 2014 15:23

[SPARK-1779] Throw an exception if memory fractions are not between 0 and 1 #714

[SPARK-1779] Throw an exception if memory fractions are not between 0 and 1 #714

Conversation

scwf commented May 9, 2014

AmplabJenkins commented May 9, 2014

andrewor14 commented May 9, 2014

andrewor14 commented May 9, 2014

scwf commented May 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewor14 commented May 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witgo commented May 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwendell commented May 10, 2014

scwf commented May 11, 2014

erikerlandson commented May 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwendell commented May 28, 2014

Choose a reason for hiding this comment

andrewor14 commented Jun 16, 2014

pwendell commented Jun 21, 2014

scwf commented Aug 4, 2014

andrewor14 commented Aug 4, 2014

andrewor14 commented Aug 4, 2014

SparkQA commented Aug 4, 2014

SparkQA commented Aug 4, 2014

Choose a reason for hiding this comment

andrewor14 commented Aug 4, 2014

scwf commented Aug 5, 2014

andrewor14 commented Aug 5, 2014

andrewor14 commented Aug 5, 2014

SparkQA commented Aug 5, 2014

pwendell commented Aug 5, 2014

SparkQA commented Aug 5, 2014

scwf commented Aug 5, 2014

andrewor14 commented Aug 5, 2014

pwendell commented Aug 5, 2014

SparkQA commented Aug 5, 2014

SparkQA commented Aug 5, 2014

pwendell commented Aug 5, 2014