Skip to content

Commit

Permalink
Review feedback round 1.
Browse files Browse the repository at this point in the history
  • Loading branch information
Marcelo Vanzin committed Jan 14, 2015
1 parent fc6a3e2 commit c617539
Show file tree
Hide file tree
Showing 12 changed files with 305 additions and 309 deletions.
1 change: 0 additions & 1 deletion bin/pyspark2.cmd
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,4 @@ set PYTHONPATH=%SPARK_HOME%\python\lib\py4j-0.8.2.1-src.zip;%PYTHONPATH%
set OLD_PYTHONSTARTUP=%PYTHONSTARTUP%
set PYTHONSTARTUP=%SPARK_HOME%\python\pyspark\shell.py

echo Running %PYSPARK_PYTHON% with PYTHONPATH=%PYTHONPATH%
call %SPARK_HOME%\bin\spark-class2.cmd pyspark %*
2 changes: 1 addition & 1 deletion core/src/main/scala/org/apache/spark/deploy/worker/CommandUtils.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ private class CommandLauncher(sparkHome: String, memory: Int, env: Map[String, S
setSparkHome(sparkHome)

override def buildLauncherCommand(): JList[String] = {
val cmd = createJavaCommand()
val cmd = buildJavaCommand()
cmd.add("-cp")
cmd.add(buildClassPath(null).mkString(File.pathSeparator))
cmd.add(s"-Xms${memory}M")
Expand Down
223 changes: 51 additions & 172 deletions launcher/src/main/java/org/apache/spark/launcher/AbstractLauncher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,11 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Enumeration;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
import java.util.regex.Pattern;

Expand All @@ -39,6 +38,7 @@
*/
public abstract class AbstractLauncher<T extends AbstractLauncher> extends LauncherCommon {

private static final String DEFAULT_PROPERTIES_FILE = "spark-defaults.conf";
protected static final String DEFAULT_MEM = "512m";

protected String javaHome;
Expand Down Expand Up @@ -93,18 +93,19 @@ public T setConf(String key, String value) {
*/
protected abstract List<String> buildLauncherCommand() throws IOException;

/**
* Loads the configuration file for the application, if it exists. This is either the
* user-specified properties file, or the spark-defaults.conf file under the Spark configuration
* directory.
*/
protected Properties loadPropertiesFile() throws IOException {
Properties props = new Properties();
File propsFile;
if (propertiesFile != null) {
propsFile = new File(propertiesFile);
checkArgument(propsFile.isFile(), "Invalid properties file '%s'.", propertiesFile);
} else {
String confDir = getenv("SPARK_CONF_DIR");
if (confDir == null) {
confDir = join(File.separator, getSparkHome(), "conf");
}
propsFile = new File(confDir, "spark-defaults.conf");
propsFile = new File(getConfDir(), DEFAULT_PROPERTIES_FILE);
}

if (propsFile.isFile()) {
Expand All @@ -127,16 +128,16 @@ protected Properties loadPropertiesFile() throws IOException {
}

protected String getSparkHome() {
String path = first(sparkHome, getenv("SPARK_HOME"));
String path = firstNonEmpty(sparkHome, getenv("SPARK_HOME"));
checkState(path != null,
"Spark home not found; set it explicitly or use the SPARK_HOME environment variable.");
"Spark home not found; set it explicitly or use the SPARK_HOME environment variable.");
return path;
}

protected List<String> createJavaCommand() throws IOException {
protected List<String> buildJavaCommand() throws IOException {
List<String> cmd = new ArrayList<String>();
if (javaHome == null) {
cmd.add(join(File.separator, System.getProperty("java.home"), "..", "bin", "java"));
cmd.add(join(File.separator, System.getProperty("java.home"), "bin", "java"));
} else {
cmd.add(join(File.separator, javaHome, "bin", "java"));
}
Expand Down Expand Up @@ -186,12 +187,7 @@ protected List<String> buildClassPath(String appClassPath) throws IOException {
addToClassPath(cp, getenv("SPARK_CLASSPATH"));
addToClassPath(cp, appClassPath);

String confDir = getenv("SPARK_CONF_DIR");
if (!isEmpty(confDir)) {
addToClassPath(cp, confDir);
} else {
addToClassPath(cp, join(File.separator, getSparkHome(), "conf"));
}
addToClassPath(cp, getConfDir());

boolean prependClasses = !isEmpty(getenv("SPARK_PREPEND_CLASSES"));
boolean isTesting = "1".equals(getenv("SPARK_TESTING"));
Expand Down Expand Up @@ -236,7 +232,7 @@ protected List<String> buildClassPath(String appClassPath) throws IOException {
} catch (IOException ioe) {
if (ioe.getMessage().indexOf("invalid CEN header") > 0) {
System.err.println(
"Loading Spark jar with '$JAR_CMD' failed.\n" +
"Loading Spark jar failed.\n" +
"This is likely because Spark was compiled with Java 7 and run\n" +
"with Java 6 (see SPARK-1703). Please use Java 7 to run Spark\n" +
"or build Spark with Java 6.");
Expand Down Expand Up @@ -279,6 +275,12 @@ protected List<String> buildClassPath(String appClassPath) throws IOException {
return cp;
}

/**
* Adds entries to the classpath.
*
* @param cp List where to appended the new classpath entries.
* @param entries New classpath entries (separated by File.pathSeparator).
*/
private void addToClassPath(List<String> cp, String entries) {
if (isEmpty(entries)) {
return;
Expand Down Expand Up @@ -317,7 +319,14 @@ protected String getScalaVersion() {
throw new IllegalStateException("Should not reach here.");
}

protected List<String> prepareForOs(List<String> cmd, String libPath) {
return prepareForOs(cmd, libPath, Collections.<String, String>emptyMap());
}

/**
* Prepare the command for execution under the current OS, setting the passed environment
* variables.
*
* Which OS is running defines two things:
* - the name of the environment variable used to define the lookup path for native libs
* - how to execute the command in general.
Expand All @@ -329,7 +338,8 @@ protected String getScalaVersion() {
*
* For Win32, see {@link #prepareForWindows(List<String>,String)}.
*/
protected List<String> prepareForOs(List<String> cmd,
protected List<String> prepareForOs(
List<String> cmd,
String libPath,
Map<String, String> env) {

Expand Down Expand Up @@ -365,128 +375,6 @@ protected List<String> prepareForOs(List<String> cmd,
return newCmd;
}

protected String shQuote(String s) {
StringBuilder quoted = new StringBuilder();
boolean hasWhitespace = false;
for (int i = 0; i < s.length(); i++) {
if (Character.isWhitespace(s.codePointAt(i))) {
quoted.append('"');
hasWhitespace = true;
break;
}
}

for (int i = 0; i < s.length(); i++) {
int cp = s.codePointAt(i);
switch (cp) {
case '\'':
if (hasWhitespace) {
quoted.appendCodePoint(cp);
break;
}
case '"':
case '\\':
quoted.append('\\');
// Fall through.
default:
if (Character.isWhitespace(cp)) {
hasWhitespace=true;
}
quoted.appendCodePoint(cp);
}
}
if (hasWhitespace) {
quoted.append('"');
}
return quoted.toString();
}

// Visible for testing.
List<String> parseOptionString(String s) {
List<String> opts = new ArrayList<String>();
StringBuilder opt = new StringBuilder();
boolean inOpt = false;
boolean inSingleQuote = false;
boolean inDoubleQuote = false;
boolean escapeNext = false;
boolean hasData = false;

for (int i = 0; i < s.length(); i++) {
int c = s.codePointAt(i);
if (escapeNext) {
if (!inOpt) {
inOpt = true;
}
opt.appendCodePoint(c);
escapeNext = false;
} else if (inOpt) {
switch (c) {
case '\\':
if (inSingleQuote) {
opt.appendCodePoint(c);
} else {
escapeNext = true;
}
break;
case '\'':
if (inDoubleQuote) {
opt.appendCodePoint(c);
} else {
inSingleQuote = !inSingleQuote;
}
break;
case '"':
if (inSingleQuote) {
opt.appendCodePoint(c);
} else {
inDoubleQuote = !inDoubleQuote;
}
break;
default:
if (inSingleQuote || inDoubleQuote || !Character.isWhitespace(c)) {
opt.appendCodePoint(c);
} else {
finishOpt(opts, opt);
inOpt = false;
hasData = false;
}
}
} else {
switch (c) {
case '\'':
inSingleQuote = true;
inOpt = true;
hasData = true;
break;
case '"':
inDoubleQuote = true;
inOpt = true;
hasData = true;
break;
case '\\':
escapeNext = true;
break;
default:
if (!Character.isWhitespace(c)) {
inOpt = true;
opt.appendCodePoint(c);
}
}
}
}

checkArgument(!inSingleQuote && !inDoubleQuote && !escapeNext, "Invalid option string: %s", s);
if (opt.length() > 0 || hasData) {
opts.add(opt.toString());
}
return opts;
}

private void finishOpt(List<String> opts, StringBuilder opt) {
opts.add(opt.toString());
opt.setLength(0);
}

private String findAssembly(String scalaVersion) {
String sparkHome = getSparkHome();
File libdir;
Expand All @@ -512,7 +400,12 @@ public boolean accept(File file) {
}

private String getenv(String key) {
return first(env != null ? env.get(key) : null, System.getenv(key));
return firstNonEmpty(env != null ? env.get(key) : null, System.getenv(key));
}

private String getConfDir() {
String confDir = getenv("SPARK_CONF_DIR");
return confDir != null ? confDir : join(File.separator, getSparkHome(), "conf");
}

/**
Expand All @@ -526,43 +419,48 @@ private String getenv(String key) {
* - Quote all arguments so that spaces are handled as expected. Quotes within arguments are
* "double quoted" (which is batch for escaping a quote). This page has more details about
* quoting and other batch script fun stuff: http://ss64.com/nt/syntax-esc.html
*
* The command is executed using "cmd /c" and formatted in a single line, since that's the
* easiest way to consume this from a batch script (see spark-class2.cmd).
*/
private List<String> prepareForWindows(List<String> cmd,
private List<String> prepareForWindows(
List<String> cmd,
String libPath,
Map<String, String> env) {
StringBuilder cmdline = new StringBuilder("cmd /c \"");
if (libPath != null) {
cmdline.append("set PATH=%PATH%;").append(libPath).append(" &&");
}
for (Map.Entry<String, String> e : env.entrySet()) {
if (cmdline.length() > 0) {
cmdline.append(" ");
}
cmdline.append(String.format("set %s=%s", e.getKey(), e.getValue()));
cmdline.append(" &&");
}
for (String arg : cmd) {
if (cmdline.length() > 0) {
cmdline.append(" ");
}
cmdline.append(quote(arg));
cmdline.append(quoteForBatchScript(arg));
}
cmdline.append("\"");
return Arrays.asList(cmdline.toString());
}

/**
* Quoting arguments that don't need quoting in Windows seems to cause weird issues. So only
* quote arguments when there is whitespace in them.
* Quote a command argument for a command to be run by a Windows batch script, if the argument
* needs quoting. Arguments only seem to need quotes in batch scripts if they have whitespace.
*/
private boolean needsQuoting(String arg) {
private String quoteForBatchScript(String arg) {
boolean needsQuotes = false;
for (int i = 0; i < arg.length(); i++) {
if (Character.isWhitespace(arg.codePointAt(i))) {
return true;
needsQuotes = true;
break;
}
}
return false;
}

private String quote(String arg) {
if (!needsQuoting(arg)) {
if (!needsQuotes) {
return arg;
}
StringBuilder quoted = new StringBuilder();
Expand All @@ -578,23 +476,4 @@ private String quote(String arg) {
return quoted.toString();
}

// Visible for testing.
String getLibPathEnvName() {
if (isWindows()) {
return "PATH";
}

String os = System.getProperty("os.name");
if (os.startsWith("Mac OS X")) {
return "DYLD_LIBRARY_PATH";
} else {
return "LD_LIBRARY_PATH";
}
}

protected boolean isWindows() {
String os = System.getProperty("os.name");
return os.startsWith("Windows");
}

}
Loading

0 comments on commit c617539

Please sign in to comment.