Incorporate review comments

- group common args for mariadb and non-mariadb
- raise exception from mariadb specific methods when called by non-mariadb
- remove un-necessary separation between mariadb and non-mariadb

Author: amitsadaphule

testing-mysql-server-5/src/main/java/com/facebook/presto/testing/mysql/EmbeddedMySql5.java

@@ -13,9 +13,11 @@
  */
 package com.facebook.presto.testing.mysql;
 
+import com.google.common.base.VerifyException;
 import com.google.common.collect.ImmutableList;
 
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.List;
 
 final class EmbeddedMySql5
@@ -30,60 +32,48 @@ public EmbeddedMySql5(MySqlOptions mySqlOptions)
     @Override
     public List<String> getInitializationArguments()
     {
-        if (isMariadb) {
-            return ImmutableList.of(
+        List<String> list = Arrays.asList(
                 "--no-defaults",
-                "--basedir=" + getBaseDirectory(),
-                "--datadir=" + getDataDirectory(),
                 "--skip-sync-frm",
-                "--innodb-flush-method=nosync");
+                "--innodb-flush-method=nosync",
+                "--datadir=" + getDataDirectory());
+
+        if (isMariadb) {
+            return ImmutableList.<String>builder().addAll(list).add("--basedir=" + getBaseDirectory()).build();
         }
         else {
-            return ImmutableList.of(
-                    "--no-defaults",
-                    "--initialize-insecure",
-                    "--skip-sync-frm",
-                    "--innodb-flush-method=nosync",
-                    "--datadir", getDataDirectory());
+            return ImmutableList.<String>builder().addAll(list).add("--initialize-insecure").build();
         }
     }
 
     @Override
-    public List<String> getStartArguments()
+    public List<String> getStartArguments() throws VerifyException
     {
+        List<String> list = Arrays.asList(
+                "--no-defaults",
+                "--default-time-zone=+00:00",
+                "--skip-sync-frm",
+                "--innodb-flush-method=nosync",
+                "--innodb-flush-log-at-trx-commit=0",
+                "--innodb-doublewrite=0",
+                "--bind-address=localhost",
+                "--port=" + String.valueOf(getPort()),
+                "--datadir=" + getDataDirectory(),
+                "--socket=" + getSocketDirectory());
+
         if (isMariadb) {
-            return ImmutableList.of(
-                    "--no-defaults",
-                    "--default-time-zone=+00:00",
-                    "--skip-sync-frm",
-                    "--innodb-flush-method=nosync",
-                    "--innodb-flush-log-at-trx-commit=0",
-                    "--innodb-doublewrite=0",
-                    "--bind-address=localhost",
-                    "--basedir=" + getBaseDirectory(),
-                    "--plugin-dir=" + getPluginDirectory(),
-                    "--log-error=" + getDataDirectory() + "mariadb.log",
-                    "--pid-file=" + getDataDirectory() + "mariadb.pid",
-                    "--socket=" + getDataDirectory() + "mysql.sock",
-                    "--port=" + String.valueOf(getPort()),
-                    "--datadir=" + getDataDirectory());
+            return ImmutableList.<String>builder().addAll(list).add(
+                "--basedir=" + getBaseDirectory(),
+                "--plugin-dir=" + getMariadbPluginDirectory(),
+                "--log-error=" + getDataDirectory() + "mariadb.log",
+                "--pid-file=" + getDataDirectory() + "mariadb.pid").build();
         }
         else {
-            return ImmutableList.of(
-                    "--no-defaults",
-                    "--skip-ssl",
-                    "--default-time-zone=+00:00",
-                    "--disable-partition-engine-check",
-                    "--explicit_defaults_for_timestamp",
-                    "--skip-sync-frm",
-                    "--innodb-flush-method=nosync",
-                    "--innodb-flush-log-at-trx-commit=0",
-                    "--innodb-doublewrite=0",
-                    "--bind-address=localhost",
-                    "--lc_messages_dir", getShareDirectory(),
-                    "--socket", getSocketDirectory(),
-                    "--port", String.valueOf(getPort()),
-                    "--datadir", getDataDirectory());
+            return ImmutableList.<String>builder().addAll(list).add(
+                "--skip-ssl",
+                "--disable-partition-engine-check",
+                "--explicit_defaults_for_timestamp",
+                "--lc_messages_dir=" + getShareDirectory()).build();
         }
     }
 }

testing-mysql-server-base/src/main/java/com/facebook/presto/testing/mysql/AbstractEmbeddedMySql.java

@@ -14,6 +14,7 @@
 package com.facebook.presto.testing.mysql;
 
 import com.facebook.airlift.log.Logger;
+import com.google.common.base.VerifyException;
 import com.google.common.collect.ImmutableList;
 import com.google.common.io.ByteStreams;
 import io.airlift.command.Command;
@@ -72,7 +73,7 @@ public abstract class AbstractEmbeddedMySql
     protected final boolean isMariadb = System.getProperty("os.arch").equals("ppc64le");
 
     public AbstractEmbeddedMySql(MySqlOptions mySqlOptions)
-            throws IOException
+            throws IOException, VerifyException
     {
         this.startupWait = requireNonNull(mySqlOptions.getStartupWait(), "startupWait is null");
         this.shutdownWait = requireNonNull(mySqlOptions.getShutdownWait(), "shutdownWait is null");
@@ -113,9 +114,12 @@ public Connection getMySqlDatabase()
         return DriverManager.getConnection(getJdbcUrl("root", "mysql"));
     }
 
-    protected String getMysqlInstallDb()
+    protected String getMariadbInstallDb() throws VerifyException
     {
-        return (isMariadb ? serverDirectory.resolve("bin").resolve("mysql_install_db").toString() : "");
+        if (!isMariadb) {
+            throw new VerifyException("mysql_install_db not applicable to non-mariadb installations");
+        }
+        return serverDirectory.resolve("bin").resolve("mysql_install_db").toString();
     }
 
     protected String getBaseDirectory()
@@ -140,12 +144,15 @@ protected String getShareDirectory()
 
     protected String getSocketDirectory()
     {
-        return (isMariadb ? "" : serverDirectory.resolve("mysql.sock").toString());
+        return (isMariadb ? getDataDirectory() + "mysql.sock" : serverDirectory.resolve("mysql.sock").toString());
     }
 
-    protected String getPluginDirectory()
+    protected String getMariadbPluginDirectory() throws VerifyException
     {
-        return (isMariadb ? serverDirectory.resolve("lib64").resolve("mysql").resolve("plugin").toString() : "");
+        if (!isMariadb) {
+            throw new VerifyException("--plugin-dir option not applicable to non-mariadb installations");
+        }
+        return serverDirectory.resolve("lib64").resolve("mysql").resolve("plugin").toString();
     }
 
     @Override
@@ -199,11 +206,11 @@ private static int randomPort()
         }
     }
 
-    private void initialize()
+    private void initialize() throws VerifyException
     {
         if (isMariadb) {
             system(ImmutableList.<String>builder()
-                    .add(getMysqlInstallDb())
+                    .add(getMariadbInstallDb())
                     .addAll(getInitializationArguments())
                     .build());
         }
@@ -216,7 +223,7 @@ private void initialize()
     }
 
     private Process startMysqld()
-            throws IOException
+            throws IOException, VerifyException
     {
         Process process = new ProcessBuilder(ImmutableList.<String>builder().add(getMysqld()).addAll(getStartArguments()).build())
                 .redirectErrorStream(true)

testing-mysql-server-base/src/main/java/com/facebook/presto/testing/mysql/AbstractTestingMySqlServer.java

@@ -53,8 +53,8 @@ public AbstractTestingMySqlServer(EmbeddedMySql server, String user, String pass
         try (Connection connection = server.getMySqlDatabase()) {
             version = connection.getMetaData().getDatabaseProductVersion();
             try (Statement statement = connection.createStatement()) {
-                execute(statement, format("CREATE USER '%s'@'%s' IDENTIFIED BY '%s'", user, isMariadb ? "localhost" : "%%", password));
-                execute(statement, format("GRANT ALL ON *.* to '%s'@'%s' WITH GRANT OPTION", user, isMariadb ? "localhost" : "%%"));
+                execute(statement, format("CREATE USER '%s'@'localhost' IDENTIFIED BY '%s'", user, password));
+                execute(statement, format("GRANT ALL ON *.* to '%s'@'localhost' WITH GRANT OPTION", user));
                 for (String database : databases) {
                     execute(statement, format("CREATE DATABASE %s", database));
                 }