Allow both Java system properties and environment variables for HiveE…

…xternalWorkerQueryRunner

The documentation https://github.com/prestodb/presto/tree/master/presto
-native-execution#development says the DATA_DIR needs to be set as an
environment variable, however, the dataDirectory in createJavaQueryRunner()
was assigned to System.getProperty("DATA_DIR"), which gets null and causes
NullPointerException. Furthermore, System.getProperty() and System.getenv()
were used randomly in the native worker query runners. This is confusing
and could cause more future issues. System.getProperty() reads the Java
system properties that are passed in using -Dkey=value, while System.getenv()
reads environment variables. They cannot be used interchangably and would
return null if the corresponding values are not set. While the documentation
uses environment variables, CircleCI jobs rely on system variables. In order
to make the query runners more robust, it's better to support both of them.
This commit introduces HiveTestUtiles.getProperty() static method to read
both. If both system property and environment variable are not set, an
exception would be thrown. If both of them are set but the values are
different, an exception would also be thrown.

presto-hive/src/test/java/com/facebook/presto/hive/HiveTestUtils.java

@@ -77,12 +77,14 @@
 
 import java.math.BigDecimal;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 
 import static com.facebook.airlift.json.JsonCodec.jsonCodec;
 import static com.facebook.airlift.json.smile.SmileCodec.smileCodec;
 import static com.facebook.presto.common.type.Decimals.encodeScaledValue;
 import static com.facebook.presto.hive.HiveDwrfEncryptionProvider.NO_ENCRYPTION;
+import static java.lang.String.format;
 import static java.util.stream.Collectors.toList;
 
 public final class HiveTestUtils
@@ -267,4 +269,28 @@ public static Slice longDecimal(String value)
     {
         return encodeScaledValue(new BigDecimal(value));
     }
+
+    public static Optional<String> getProperty(String name)
+    {
+        String systemPropertyValue = System.getProperty(name);
+        String environmentVariableValue = System.getenv(name);
+        if (systemPropertyValue == null) {
+            if (environmentVariableValue == null) {
+                return Optional.empty();
+            }
+            else {
+                return Optional.of(environmentVariableValue);
+            }
+        }
+        else {
+            if (environmentVariableValue != null && !systemPropertyValue.equals(environmentVariableValue)) {
+                throw new IllegalArgumentException(format("%s is set in both Java system property and environment variable, but their values are different. The Java system property value is %s, while the" +
+                                " environment variable value is %s. Please use only one value.",
+                        name,
+                        systemPropertyValue,
+                        environmentVariableValue));
+            }
+            return Optional.of(systemPropertyValue);
+        }
+    }
 }

presto-native-execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeRemoteFunctions.java

@@ -18,12 +18,12 @@
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
+import static com.facebook.presto.hive.HiveTestUtils.getProperty;
 import static com.facebook.presto.nativeworker.NativeQueryRunnerUtils.createLineitem;
 import static com.facebook.presto.nativeworker.PrestoNativeQueryRunnerUtils.REMOTE_FUNCTION_CATALOG_NAME;
 import static com.facebook.presto.nativeworker.PrestoNativeQueryRunnerUtils.REMOTE_FUNCTION_JSON_SIGNATURES;
 import static com.facebook.presto.nativeworker.PrestoNativeQueryRunnerUtils.setupJsonFunctionNamespaceManager;
 import static com.facebook.presto.nativeworker.PrestoNativeQueryRunnerUtils.startRemoteFunctionServer;
-import static java.util.Objects.requireNonNull;
 
 @Test(groups = "remote-function")
 public abstract class AbstractTestNativeRemoteFunctions
@@ -49,12 +49,8 @@ protected void preInitQueryRunners()
     {
         // Initialize the remote function thrift server process.
         String propertyName = "REMOTE_FUNCTION_SERVER";
-        remoteFunctionServerBinaryPath = System.getenv(propertyName);
-        if (remoteFunctionServerBinaryPath == null) {
-            remoteFunctionServerBinaryPath = System.getProperty(propertyName);
-        }
-
-        requireNonNull(remoteFunctionServerBinaryPath, "Remote function server path missing. Add -DREMOTE_FUNCTION_SERVER=<path/to/binary> to your JVM arguments.");
+        remoteFunctionServerBinaryPath = getProperty(propertyName).orElseThrow(() -> new NullPointerException("Remote function server path missing. Add -DREMOTE_FUNCTION_SERVER=<path/to/binary>" +
+                " to your JVM arguments, or create an environment variable REMOTE_FUNCTION_SERVER with value <path/to/binary>."));
         remoteFunctionServerUds = startRemoteFunctionServer(remoteFunctionServerBinaryPath);
     }
 

presto-native-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java

@@ -31,6 +31,7 @@
 import java.util.Optional;
 import java.util.UUID;
 
+import static com.facebook.presto.hive.HiveTestUtils.getProperty;
 import static com.facebook.presto.nativeworker.NativeQueryRunnerUtils.getNativeWorkerHiveProperties;
 import static com.facebook.presto.nativeworker.NativeQueryRunnerUtils.getNativeWorkerSystemProperties;
 import static java.lang.String.format;
@@ -39,15 +40,14 @@
 
 public class PrestoNativeQueryRunnerUtils
 {
-    private static final Logger log = Logger.get(PrestoNativeQueryRunnerUtils.class);
-
-    private static final String DEFAULT_STORAGE_FORMAT = "DWRF";
-
     // The unix domain socket (UDS) used to communicate with the remote function server.
     public static final String REMOTE_FUNCTION_UDS = "remote_function_server.socket";
     public static final String REMOTE_FUNCTION_JSON_SIGNATURES = "remote_function_server.json";
     public static final String REMOTE_FUNCTION_CATALOG_NAME = "remote";
 
+    private static final Logger log = Logger.get(PrestoNativeQueryRunnerUtils.class);
+    private static final String DEFAULT_STORAGE_FORMAT = "DWRF";
+
     private PrestoNativeQueryRunnerUtils() {}
 
     public static QueryRunner createQueryRunner()
@@ -250,11 +250,11 @@ public static String startRemoteFunctionServer(String remoteFunctionServerBinary
             log.info("Temp directory for Remote Function Server: %s", tempDirectoryPath.toString());
 
             Process p = new ProcessBuilder(Paths.get(remoteFunctionServerBinaryPath).toAbsolutePath().toString(), "--uds_path", remoteFunctionServerUdsPath.toString(), "--function_prefix", REMOTE_FUNCTION_CATALOG_NAME + ".schema.")
-                                .directory(tempDirectoryPath.toFile())
-                                .redirectErrorStream(true)
-                                .redirectOutput(ProcessBuilder.Redirect.to(tempDirectoryPath.resolve("thrift_server.out").toFile()))
-                                .redirectError(ProcessBuilder.Redirect.to(tempDirectoryPath.resolve("thrift_server.err").toFile()))
-                                .start();
+                    .directory(tempDirectoryPath.toFile())
+                    .redirectErrorStream(true)
+                    .redirectOutput(ProcessBuilder.Redirect.to(tempDirectoryPath.resolve("thrift_server.out").toFile()))
+                    .redirectError(ProcessBuilder.Redirect.to(tempDirectoryPath.resolve("thrift_server.err").toFile()))
+                    .start();
             return remoteFunctionServerUdsPath.toString();
         }
         catch (IOException e) {
@@ -264,19 +264,13 @@ public static String startRemoteFunctionServer(String remoteFunctionServerBinary
 
     public static NativeQueryRunnerParameters getNativeQueryRunnerParameters()
     {
-        Path prestoServerPath = Paths.get(Optional.ofNullable(System.getenv("PRESTO_SERVER"))
-                        .orElseGet(() -> Optional.ofNullable(System.getProperty("PRESTO_SERVER"))
-                                .orElse("_build/debug/presto_cpp/main/presto_server")))
+        Path prestoServerPath = Paths.get(getProperty("PRESTO_SERVER")
+                .orElse("_build/debug/presto_cpp/main/presto_server"))
                 .toAbsolutePath();
-        Path dataDirectory = Paths.get(Optional.ofNullable(System.getenv("DATA_DIR"))
-                        .orElseGet(() -> Optional.ofNullable(System.getProperty("DATA_DIR"))
-                                .orElse("target/velox_data")))
+        Path dataDirectory = Paths.get(getProperty("DATA_DIR")
+                .orElse("target/velox_data"))
                 .toAbsolutePath();
-        Optional<String> workerProp = Optional.ofNullable(System.getenv("WORKER_COUNT"));
-        if (!workerProp.isPresent()) {
-            workerProp = Optional.ofNullable(System.getProperty("WORKER_COUNT"));
-        }
-        Optional<Integer> workerCount = workerProp.map(Integer::parseInt);
+        Optional<Integer> workerCount = getProperty("WORKER_COUNT").map(Integer::parseInt);
 
         assertTrue(Files.exists(prestoServerPath), format("Native worker binary at %s not found. Add -DPRESTO_SERVER=<path/to/presto_server> to your JVM arguments.", prestoServerPath));
         log.info("Using PRESTO_SERVER binary at %s", prestoServerPath);

presto-native-execution/src/test/java/com/facebook/presto/spark/PrestoSparkNativeQueryRunnerUtils.java

@@ -39,6 +39,7 @@
 import java.util.Set;
 
 import static com.facebook.airlift.log.Level.WARN;
+import static com.facebook.presto.hive.HiveTestUtils.getProperty;
 import static com.facebook.presto.nativeworker.NativeQueryRunnerUtils.getNativeWorkerHiveProperties;
 import static com.facebook.presto.nativeworker.NativeQueryRunnerUtils.getNativeWorkerSystemProperties;
 import static com.facebook.presto.nativeworker.PrestoNativeQueryRunnerUtils.getNativeQueryRunnerParameters;
@@ -207,8 +208,9 @@ public static synchronized Path getBaseDataPath()
         if (dataDirectory.isPresent()) {
             return dataDirectory.get();
         }
-        String dataDirectoryStr = System.getProperty("DATA_DIR");
-        if (dataDirectoryStr == null || dataDirectoryStr.isEmpty()) {
+
+        Optional<String> dataDirectoryStr = getProperty("DATA_DIR");
+        if (!dataDirectoryStr.isPresent()) {
             try {
                 dataDirectory = Optional.of(createTempDirectory("PrestoTest").toAbsolutePath());
             }