Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ package org.apache.spark.deploy.worker

import java.lang.management.ManagementFactory

import org.apache.spark.util.{IntParam, MemoryParam, Utils}
import org.apache.spark.SparkConf
import org.apache.spark.util.{IntParam, MemoryParam, Utils}

/**
* Command-line parser for the worker.
Expand Down Expand Up @@ -56,6 +56,8 @@ private[spark] class WorkerArguments(args: Array[String], conf: SparkConf) {

parse(args.toList)

checkWorkerMemory()

def parse(args: List[String]): Unit = args match {
case ("--ip" | "-i") :: value :: tail =>
Utils.checkHost(value, "ip no longer supported, please use hostname " + value)
Expand Down Expand Up @@ -153,4 +155,11 @@ private[spark] class WorkerArguments(args: Array[String], conf: SparkConf) {
// Leave out 1 GB for the operating system, but don't return a negative memory size
math.max(totalMb - 1024, 512)
}

def checkWorkerMemory(): Unit = {
if (memory <= 0) {
val message = "Memory can't be 0, missing a M or G on the end of the memory specification?"
throw new IllegalStateException(message)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/


package org.apache.spark.deploy.worker

import org.apache.spark.SparkConf
import org.scalatest.FunSuite


class WorkerArgumentsTest extends FunSuite {

test("Memory can't be set to 0 when cmd line args leave off M or G") {
val conf = new SparkConf
val args = Array("-m", "10000", "spark://localhost:0000 ")
intercept[IllegalStateException] {
new WorkerArguments(args, conf)
}
}


/* For this test an environment property for SPARK_WORKER_MEMORY was set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #2002, I added a mechanism that allows environment variables to be mocked in tests. Take a look at that PR, SparkConf.getEnv in particular. By using a custom SparkConf subclass, you can mock environment variables on a per-test basis: https://github.com/apache/spark/pull/2002/files#diff-e9fb6be5f96766cce96c4d60aea2fc59R45

If we find ourselves doing this in multiple places (my PR, here, ...) it might be nice to add some test helper classes for doing this more generically. That refactoring can happen in a separate PR, though, so for now it's probably fine to just copy my code snippet here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, to be more specific: you'll have to change the code that reads the environment variable to use SparkConf.getEnv instead of System.getEnv; I only changed this for the environment variables used in my specific test because I didn't want to make a big cross-cutting change across the codebase (plus it would probably get broken by subsequent PRs; we should add a style checker rule that complains about System.getEnv uses if we plan on doing this change globally).

in the scalatest-maven-plugin under the <environmentVariables> and then
run manually
<SPARK_WORKER_MEMORY>100000</SPARK_WORKER_MEMORY>
*/

// test("Memory can't be set to 0 when SPARK_WORKER_MEMORY env property leaves off M or G") {
// val conf = new SparkConf
// val args = Array("spark://localhost:0000 ")
//
// intercept[IllegalStateException] {
// new WorkerArguments(args, conf)
// }
// }

test("Check memory correctly set from args with M appended to memory value") {
val conf = new SparkConf
val args = Array("-m", "10000M", "spark://localhost:0000 ")

val workerArgs = new WorkerArguments(args, conf)
assert(workerArgs.memory === 10000)

}

}