Internal: Refactor SettingCommand into EnvironmentAwareCommand (#22175)

* Internal: Refactor SettingCommand into EnvironmentAwareCommand

This change renames and changes the behavior of SettingCommand to have
its primary method take in a fully initialized Environment for
elasticsearch instead of just a map of settings. All of the subclasses
of SettingCommand already did this at some point, so this just removes
duplication.

Author: rjernst

core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java

@@ -58,9 +58,7 @@
 import java.nio.file.Path;
 import java.security.NoSuchAlgorithmException;
 import java.util.List;
-import java.util.Locale;
-import java.util.Map;
-import java.util.Objects;
+import java.util.Collections;
 import java.util.concurrent.CountDownLatch;
 
 /**
@@ -226,13 +224,14 @@ protected void validateNodeBeforeAcceptingRequests(
         };
     }
 
-    private static Environment initialEnvironment(boolean foreground, Path pidFile, Map<String, String> esSettings) {
+    private static Environment initialEnvironment(boolean foreground, Path pidFile, Settings initialSettings) {
         Terminal terminal = foreground ? Terminal.DEFAULT : null;
         Settings.Builder builder = Settings.builder();
         if (pidFile != null) {
             builder.put(Environment.PIDFILE_SETTING.getKey(), pidFile);
         }
-        return InternalSettingsPreparer.prepareEnvironment(builder.build(), terminal, esSettings);
+        builder.put(initialSettings);
+        return InternalSettingsPreparer.prepareEnvironment(builder.build(), terminal, Collections.emptyMap());
     }
 
     private void start() throws NodeValidationException {
@@ -262,7 +261,7 @@ static void init(
             final boolean foreground,
             final Path pidFile,
             final boolean quiet,
-            final Map<String, String> esSettings) throws BootstrapException, NodeValidationException, UserException {
+            final Settings initialSettings) throws BootstrapException, NodeValidationException, UserException {
         // Set the system property before anything has a chance to trigger its use
         initLoggerPrefix();
 
@@ -272,7 +271,7 @@ static void init(
 
         INSTANCE = new Bootstrap();
 
-        Environment environment = initialEnvironment(foreground, pidFile, esSettings);
+        Environment environment = initialEnvironment(foreground, pidFile, initialSettings);
         try {
             LogConfigurator.configure(environment);
         } catch (IOException e) {

core/src/main/java/org/elasticsearch/bootstrap/BootstrapException.java

@@ -20,14 +20,14 @@
 package org.elasticsearch.bootstrap;
 
 import java.nio.file.Path;
-import java.util.Map;
 
 /**
  * Wrapper exception for checked exceptions thrown during the bootstrap process. Methods invoked
  * during bootstrap should explicitly declare the checked exceptions that they can throw, rather
  * than declaring the top-level checked exception {@link Exception}. This exception exists to wrap
- * these checked exceptions so that {@link Bootstrap#init(boolean, Path, boolean, Map)} does not have to
- * declare all of these checked exceptions.
+ * these checked exceptions so that
+ * {@link Bootstrap#init(boolean, Path, boolean, org.elasticsearch.common.settings.Settings)}
+ * does not have to declare all of these checked exceptions.
  */
 class BootstrapException extends Exception {
 

core/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java

@@ -25,9 +25,11 @@
 import joptsimple.util.PathConverter;
 import org.elasticsearch.Build;
 import org.elasticsearch.cli.ExitCodes;
-import org.elasticsearch.cli.SettingCommand;
+import org.elasticsearch.cli.EnvironmentAwareCommand;
 import org.elasticsearch.cli.Terminal;
 import org.elasticsearch.cli.UserException;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.env.Environment;
 import org.elasticsearch.monitor.jvm.JvmInfo;
 import org.elasticsearch.node.NodeValidationException;
 
@@ -40,7 +42,7 @@
 /**
  * This class starts elasticsearch.
  */
-class Elasticsearch extends SettingCommand {
+class Elasticsearch extends EnvironmentAwareCommand {
 
     private final OptionSpecBuilder versionOption;
     private final OptionSpecBuilder daemonizeOption;
@@ -90,7 +92,7 @@ static int main(final String[] args, final Elasticsearch elasticsearch, final Te
     }
 
     @Override
-    protected void execute(Terminal terminal, OptionSet options, Map<String, String> settings) throws UserException {
+    protected void execute(Terminal terminal, OptionSet options, Environment env) throws UserException {
         if (options.nonOptionArguments().isEmpty() == false) {
             throw new UserException(ExitCodes.USAGE, "Positional arguments not allowed, found " + options.nonOptionArguments());
         }
@@ -109,16 +111,16 @@ protected void execute(Terminal terminal, OptionSet options, Map<String, String>
         final boolean quiet = options.has(quietOption);
 
         try {
-            init(daemonize, pidFile, quiet, settings);
+            init(daemonize, pidFile, quiet, env.settings());
         } catch (NodeValidationException e) {
             throw new UserException(ExitCodes.CONFIG, e.getMessage());
         }
     }
 
-    void init(final boolean daemonize, final Path pidFile, final boolean quiet, final Map<String, String> esSettings)
+    void init(final boolean daemonize, final Path pidFile, final boolean quiet, Settings initialSettings)
         throws NodeValidationException, UserException {
         try {
-            Bootstrap.init(!daemonize, pidFile, quiet, esSettings);
+            Bootstrap.init(!daemonize, pidFile, quiet, initialSettings);
         } catch (BootstrapException | RuntimeException e) {
             // format exceptions to the console in a special way
             // to avoid 2MB stacktraces from guice, etc.

core/src/main/java/org/elasticsearch/cli/SettingCommand.java renamed to core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java

@@ -22,16 +22,20 @@
 import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
 import joptsimple.util.KeyValuePair;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.env.Environment;
+import org.elasticsearch.node.internal.InternalSettingsPreparer;
 
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
 
-public abstract class SettingCommand extends Command {
+/** A cli command which requires an {@link org.elasticsearch.env.Environment} to use current paths and settings. */
+public abstract class EnvironmentAwareCommand extends Command {
 
     private final OptionSpec<KeyValuePair> settingOption;
 
-    public SettingCommand(String description) {
+    public EnvironmentAwareCommand(String description) {
         super(description);
         this.settingOption = parser.accepts("E", "Configure a setting").withRequiredArg().ofType(KeyValuePair.class);
     }
@@ -51,9 +55,15 @@ protected void execute(Terminal terminal, OptionSet options) throws Exception {
         putSystemPropertyIfSettingIsMissing(settings, "path.home", "es.path.home");
         putSystemPropertyIfSettingIsMissing(settings, "path.logs", "es.path.logs");
 
-        execute(terminal, options, settings);
+        execute(terminal, options, createEnv(terminal, settings));
     }
 
+    /** Create an {@link Environment} for the command to use. Overrideable for tests. */
+    protected Environment createEnv(Terminal terminal, Map<String, String> settings) {
+        return InternalSettingsPreparer.prepareEnvironment(Settings.EMPTY, terminal, settings);
+    }
+
+    /** Ensure the given setting exists, reading it from system properties if not already set. */
     protected static void putSystemPropertyIfSettingIsMissing(final Map<String, String> settings, final String setting, final String key) {
         final String value = System.getProperty(key);
         if (value != null) {
@@ -72,6 +82,7 @@ protected static void putSystemPropertyIfSettingIsMissing(final Map<String, Stri
         }
     }
 
-    protected abstract void execute(Terminal terminal, OptionSet options, Map<String, String> settings) throws Exception;
+    /** Execute the command with the initialized {@link Environment}. */
+    protected abstract void execute(Terminal terminal, OptionSet options, Environment env) throws Exception;
 
 }

core/src/main/java/org/elasticsearch/index/translog/TruncateTranslogCommand.java

@@ -34,10 +34,11 @@
 import org.apache.lucene.util.BytesRef;
 import org.apache.lucene.util.IOUtils;
 import org.elasticsearch.ElasticsearchException;
-import org.elasticsearch.cli.SettingCommand;
+import org.elasticsearch.cli.EnvironmentAwareCommand;
 import org.elasticsearch.cli.Terminal;
 import org.elasticsearch.common.SuppressForbidden;
 import org.elasticsearch.common.io.PathUtils;
+import org.elasticsearch.env.Environment;
 import org.elasticsearch.index.IndexNotFoundException;
 import org.elasticsearch.index.seqno.SequenceNumbersService;
 
@@ -55,7 +56,7 @@
 import java.util.Map;
 import java.util.Set;
 
-public class TruncateTranslogCommand extends SettingCommand {
+public class TruncateTranslogCommand extends EnvironmentAwareCommand {
 
     private final OptionSpec<String> translogFolder;
     private final OptionSpec<Void> batchMode;
@@ -87,7 +88,7 @@ private Path getTranslogPath(OptionSet options) {
     }
 
     @Override
-    protected void execute(Terminal terminal, OptionSet options, Map<String, String> settings) throws Exception {
+    protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
         boolean batch = options.has(batchMode);
 
         Path translogPath = getTranslogPath(options);

core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

@@ -27,7 +27,7 @@
 import org.elasticsearch.Version;
 import org.elasticsearch.bootstrap.JarHell;
 import org.elasticsearch.cli.ExitCodes;
-import org.elasticsearch.cli.SettingCommand;
+import org.elasticsearch.cli.EnvironmentAwareCommand;
 import org.elasticsearch.cli.Terminal;
 import org.elasticsearch.cli.UserException;
 import org.elasticsearch.common.collect.Tuple;
@@ -103,7 +103,7 @@
  * elasticsearch config directory, using the name of the plugin. If any files to be installed
  * already exist, they will be skipped.
  */
-class InstallPluginCommand extends SettingCommand {
+class InstallPluginCommand extends EnvironmentAwareCommand {
 
     private static final String PROPERTY_STAGING_ID = "es.plugins.staging";
 
@@ -189,18 +189,17 @@ protected void printAdditionalHelp(Terminal terminal) {
     }
 
     @Override
-    protected void execute(Terminal terminal, OptionSet options, Map<String, String> settings) throws Exception {
+    protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
         String pluginId = arguments.value(options);
         boolean isBatch = options.has(batchOption) || System.console() == null;
-        execute(terminal, pluginId, isBatch, settings);
+        execute(terminal, pluginId, isBatch, env);
     }
 
     // pkg private for testing
-    void execute(Terminal terminal, String pluginId, boolean isBatch, Map<String, String> settings) throws Exception {
+    void execute(Terminal terminal, String pluginId, boolean isBatch, Environment env) throws Exception {
         if (pluginId == null) {
             throw new UserException(ExitCodes.USAGE, "plugin id is required");
         }
-        final Environment env = InternalSettingsPreparer.prepareEnvironment(Settings.EMPTY, terminal, settings);
         // TODO: remove this leniency!! is it needed anymore?
         if (Files.exists(env.pluginsFile()) == false) {
             terminal.println("Plugins directory [" + env.pluginsFile() + "] does not exist. Creating...");

core/src/main/java/org/elasticsearch/plugins/ListPluginsCommand.java

@@ -20,7 +20,7 @@
 package org.elasticsearch.plugins;
 
 import joptsimple.OptionSet;
-import org.elasticsearch.cli.SettingCommand;
+import org.elasticsearch.cli.EnvironmentAwareCommand;
 import org.elasticsearch.cli.Terminal;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
@@ -38,15 +38,14 @@
 /**
  * A command for the plugin cli to list plugins installed in elasticsearch.
  */
-class ListPluginsCommand extends SettingCommand {
+class ListPluginsCommand extends EnvironmentAwareCommand {
 
     ListPluginsCommand() {
         super("Lists installed elasticsearch plugins");
     }
 
     @Override
-    protected void execute(Terminal terminal, OptionSet options, Map<String, String> settings) throws Exception {
-        final Environment env = InternalSettingsPreparer.prepareEnvironment(Settings.EMPTY, terminal, settings);
+    protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
         if (Files.exists(env.pluginsFile()) == false) {
             throw new IOException("Plugins directory missing: " + env.pluginsFile());
         }

core/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java

@@ -19,32 +19,28 @@
 
 package org.elasticsearch.plugins;
 
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
+import java.util.ArrayList;
+import java.util.List;
+
 import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
-
 import org.apache.lucene.util.IOUtils;
+import org.elasticsearch.cli.EnvironmentAwareCommand;
 import org.elasticsearch.cli.ExitCodes;
-import org.elasticsearch.cli.SettingCommand;
 import org.elasticsearch.cli.Terminal;
 import org.elasticsearch.cli.UserException;
 import org.elasticsearch.common.Strings;
-import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
-import org.elasticsearch.node.internal.InternalSettingsPreparer;
-
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.nio.file.StandardCopyOption;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
 
 import static org.elasticsearch.cli.Terminal.Verbosity.VERBOSE;
 
 /**
  * A command for the plugin cli to remove a plugin from elasticsearch.
  */
-class RemovePluginCommand extends SettingCommand {
+class RemovePluginCommand extends EnvironmentAwareCommand {
 
     private final OptionSpec<String> arguments;
 
@@ -54,15 +50,13 @@ class RemovePluginCommand extends SettingCommand {
     }
 
     @Override
-    protected void execute(Terminal terminal, OptionSet options, Map<String, String> settings) throws Exception {
+    protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
         String arg = arguments.value(options);
-        execute(terminal, arg, settings);
+        execute(terminal, arg, env);
     }
 
     // pkg private for testing
-    void execute(Terminal terminal, String pluginName, Map<String, String> settings) throws Exception {
-        final Environment env = InternalSettingsPreparer.prepareEnvironment(Settings.EMPTY, terminal, settings);
-
+    void execute(Terminal terminal, String pluginName, Environment env) throws Exception {
         terminal.println("-> Removing " + Strings.coalesceToEmpty(pluginName) + "...");
 
         final Path pluginDir = env.pluginsFile().resolve(pluginName);

core/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java

@@ -25,6 +25,7 @@
 import org.elasticsearch.monitor.jvm.JvmInfo;
 
 import java.nio.file.Path;
+import java.util.Map;
 import java.util.function.Consumer;
 
 import static org.hamcrest.CoreMatchers.containsString;
@@ -152,9 +153,9 @@ public void testElasticsearchSettings() throws Exception {
                 true,
                 output -> {},
                 (foreground, pidFile, quiet, esSettings) -> {
-                    assertThat(esSettings.size(), equalTo(2));
-                    assertThat(esSettings, hasEntry("foo", "bar"));
-                    assertThat(esSettings, hasEntry("baz", "qux"));
+                    Map<String, String> settings = esSettings.getAsMap();
+                    assertThat(settings, hasEntry("foo", "bar"));
+                    assertThat(settings, hasEntry("baz", "qux"));
                 },
                 "-Efoo=bar", "-E", "baz=qux"
         );

core/src/test/java/org/elasticsearch/index/translog/TruncateTranslogIT.java

@@ -43,6 +43,7 @@
 import org.elasticsearch.common.unit.ByteSizeUnit;
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.unit.TimeValue;
+import org.elasticsearch.env.Environment;
 import org.elasticsearch.index.Index;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.MockEngineFactoryPlugin;
@@ -112,7 +113,7 @@ public void testCorruptTranslogTruncation() throws Exception {
             // Try running it before the shard is closed, it should flip out because it can't acquire the lock
             try {
                 logger.info("--> running truncate while index is open on [{}]", translogDir.toAbsolutePath());
-                ttc.execute(t, options, new HashMap<String, String>());
+                ttc.execute(t, options, null /* TODO: env should be real here, and ttc should actually use it... */);
                 fail("expected the truncate command to fail not being able to acquire the lock");
             } catch (Exception e) {
                 assertThat(e.getMessage(), containsString("Failed to lock shard's directory"));
@@ -160,7 +161,7 @@ public void testCorruptTranslogTruncation() throws Exception {
 
             OptionSet options = parser.parse("-d", translogDir.toAbsolutePath().toString(), "-b");
             logger.info("--> running truncate translog command for [{}]", translogDir.toAbsolutePath());
-            ttc.execute(t, options, new HashMap<String, String>());
+            ttc.execute(t, options, null /* TODO: env should be real here, and ttc should actually use it... */);
             logger.info("--> output:\n{}", t.getOutput());
         }