Merge pull request #2533 from metamx/javaOptsArray

Allow specifying peon javaOpts as an array

docs/content/configuration/indexing-service.md

@@ -255,7 +255,8 @@ Middle managers pass their configurations down to their child peons. The middle
 |`druid.indexer.runner.compressZnodes`|Indicates whether or not the middle managers should compress Znodes.|true|
 |`druid.indexer.runner.classpath`|Java classpath for the peon.|System.getProperty("java.class.path")|
 |`druid.indexer.runner.javaCommand`|Command required to execute java.|java|
-|`druid.indexer.runner.javaOpts`|-X Java options to run the peon in its own JVM. Can be either a string or a json string list. Quotable parameters or parameters with spaces are encouraged to use json string lists|""|
+|`druid.indexer.runner.javaOpts`|*DEPRECATED* A string of -X Java options to pass to the peon's JVM. Quotable parameters or parameters with spaces are encouraged to use javaOptsArray|""|
+|`druid.indexer.runner.javaOptsArray`|A json array of strings to be passed in as options to the peon's jvm. This is additive to javaOpts and is recommended for properly handling arguments which contain quotes or spaces like `["-XX:OnOutOfMemoryError=kill -9 %p"]`|`[]`|
 |`druid.indexer.runner.maxZnodeBytes`|The maximum size Znode in bytes that can be created in Zookeeper.|524288|
 |`druid.indexer.runner.startPort`|The port that peons begin running on.|8100|
 |`druid.indexer.runner.separateIngestionEndpoint`|*Deprecated.* Use separate server and consequently separate jetty thread pool for ingesting events|false|

indexing-service/src/main/java/io/druid/indexing/overlord/ForkingTaskRunner.java

@@ -269,10 +269,8 @@ public class ForkingTaskRunner implements TaskRunner, TaskLogStreamer
                               command.add("-cp");
                               command.add(taskClasspath);
 
-                              Iterables.addAll(
+                              Iterables.addAll(command, new QuotableWhiteSpaceSplitter(config.getJavaOpts()));
-                                  command,
+                              Iterables.addAll(command, config.getJavaOptsArray());
-                                  new QuotableWhiteSpaceSplitter(config.getJavaOpts(), jsonMapper)
-                              );
 
                               // Override task specific javaOpts
                               Object taskJavaOpts = task.getContextValue(
@@ -281,7 +279,7 @@ public class ForkingTaskRunner implements TaskRunner, TaskLogStreamer
                               if (taskJavaOpts != null) {
                                 Iterables.addAll(
                                     command,
-                                    new QuotableWhiteSpaceSplitter((String) taskJavaOpts, jsonMapper)
+                                    new QuotableWhiteSpaceSplitter((String) taskJavaOpts)
                                 );
                               }
 
@@ -289,7 +287,9 @@ public class ForkingTaskRunner implements TaskRunner, TaskLogStreamer
                                 for (String allowedPrefix : config.getAllowedPrefixes()) {
                                   // See https://github.com/druid-io/druid/issues/1841
                                   if (propName.startsWith(allowedPrefix)
-                                      && !ForkingTaskRunnerConfig.JAVA_OPTS_PROPERTY.equals(propName)) {
+                                      && !ForkingTaskRunnerConfig.JAVA_OPTS_PROPERTY.equals(propName)
+                                      && !ForkingTaskRunnerConfig.JAVA_OPTS_ARRAY_PROPERTY.equals(propName)
+                                      ) {
                                     command.add(
                                         String.format(
                                             "-D%s=%s",
@@ -741,29 +741,15 @@ class QuotableWhiteSpaceSplitter implements Iterable<String>
 {
   private static final Logger LOG = new Logger(QuotableWhiteSpaceSplitter.class);
   private final String string;
-  private final ObjectMapper mapper;
 
-  public QuotableWhiteSpaceSplitter(String string, ObjectMapper jsonMapper)
+  public QuotableWhiteSpaceSplitter(String string)
   {
     this.string = Preconditions.checkNotNull(string);
-    this.mapper = jsonMapper;
   }
 
   @Override
   public Iterator<String> iterator()
   {
-    try (JsonParser parser = mapper.getFactory().createParser(string)) {
-      final JsonToken token = parser.nextToken();
-      if (JsonToken.START_ARRAY.equals(token)) {
-        return mapper.<List<String>>readValue(string, new TypeReference<List<String>>()
-        {
-        }).iterator();
-      }
-    }
-    catch (IOException e) {
-      LOG.debug(e, "Could not parse %s", string);
-    }
-    LOG.debug("Not json, hoping it is a good string : %s", string);
     return Splitter.on(
         new CharMatcher()
         {

indexing-service/src/main/java/io/druid/indexing/overlord/config/ForkingTaskRunnerConfig.java

@@ -20,6 +20,7 @@
 package io.druid.indexing.overlord.config;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import io.druid.guice.IndexingServiceModuleHelper;
 
@@ -32,6 +33,8 @@ public class ForkingTaskRunnerConfig
 {
   public static final String JAVA_OPTS_PROPERTY = IndexingServiceModuleHelper.INDEXER_RUNNER_PROPERTY_PREFIX
                                                   + ".javaOpts";
+  public static final String JAVA_OPTS_ARRAY_PROPERTY = IndexingServiceModuleHelper.INDEXER_RUNNER_PROPERTY_PREFIX
+                                                  + ".javaOptsArray";
 
   @JsonProperty
   @NotNull
@@ -46,6 +49,10 @@ public class ForkingTaskRunnerConfig
   @NotNull
   private String javaOpts = "";
 
+  @JsonProperty
+  @NotNull
+  private List<String> javaOptsArray = ImmutableList.of();
+
   @JsonProperty
   @NotNull
   private String classpath = System.getProperty("java.class.path");
@@ -70,7 +77,8 @@ public class ForkingTaskRunnerConfig
   @JsonProperty
   private boolean separateIngestionEndpoint = false;
 
-  public boolean isSeparateIngestionEndpoint() {
+  public boolean isSeparateIngestionEndpoint()
+  {
     return separateIngestionEndpoint;
   }
 
@@ -84,6 +92,11 @@ public class ForkingTaskRunnerConfig
     return javaOpts;
   }
 
+  public List<String> getJavaOptsArray()
+  {
+    return javaOptsArray;
+  }
+
   public String getClasspath()
   {
     return classpath;

indexing-service/src/test/java/io/druid/indexing/overlord/ForkingTaskRunnerTest.java

@@ -34,7 +34,6 @@ import java.util.List;
 
 public class ForkingTaskRunnerTest
 {
-  private static final ObjectMapper mapper = new DefaultObjectMapper();
   // This tests the test to make sure the test fails when it should.
   @Test(expected = AssertionError.class)
   public void testPatternMatcherFailureForJavaOptions()
@@ -51,7 +50,7 @@ public class ForkingTaskRunnerTest
   @Test
   public void testPatternMatcherLeavesUnbalancedQuoteJavaOptions()
   {
-    Assert.assertEquals("\"", Iterators.get(new QuotableWhiteSpaceSplitter("\"", mapper).iterator(), 0));
+    Assert.assertEquals("\"", Iterators.get(new QuotableWhiteSpaceSplitter("\"").iterator(), 0));
   }
 
   @Test
@@ -79,7 +78,8 @@ public class ForkingTaskRunnerTest
             "some\"strange looking\"option",
             "andOtherOptions",
             "\"\"",
-            "AndMaybeEmptyQuotes"
+            "AndMaybeEmptyQuotes",
+            "keep me around"
         }
     );
     checkValues(new String[]{"\"completely quoted\""});
@@ -96,7 +96,7 @@ public class ForkingTaskRunnerTest
   @Test
   public void testEmpty()
   {
-    Assert.assertTrue(ImmutableList.copyOf(new QuotableWhiteSpaceSplitter("", mapper)).isEmpty());
+    Assert.assertTrue(ImmutableList.copyOf(new QuotableWhiteSpaceSplitter("")).isEmpty());
   }
 
   @Test
@@ -105,8 +105,7 @@ public class ForkingTaskRunnerTest
     Assert.assertEquals(
         ImmutableList.of("start", "stop"), ImmutableList.copyOf(
             new QuotableWhiteSpaceSplitter(
-                "start\t\t\t\t \n\f\r\n \f\f \n\r\f\n\r\t stop",
+                "start\t\t\t\t \n\f\r\n \f\f \n\r\f\n\r\t stop"
-                mapper
             )
         )
     );
@@ -117,26 +116,16 @@ public class ForkingTaskRunnerTest
   {
     Assert.assertTrue(
         ImmutableList.copyOf(
-            new QuotableWhiteSpaceSplitter(" \t     \t\t\t\t \n\n \f\f \n\f\r\t", mapper)
+            new QuotableWhiteSpaceSplitter(" \t     \t\t\t\t \n\n \f\f \n\f\r\t")
         ).isEmpty()
     );
   }
 
   private static void checkValues(String[] strings)
   {
-
-    try {
     Assert.assertEquals(
         ImmutableList.copyOf(strings),
-          ImmutableList.copyOf(new QuotableWhiteSpaceSplitter(mapper.writeValueAsString(Arrays.asList(strings)), mapper))
+        ImmutableList.copyOf(new QuotableWhiteSpaceSplitter(Joiner.on(" ").join(strings)))
-      );
-    }
-    catch (JsonProcessingException e) {
-      throw Throwables.propagate(e);
-    }
-    Assert.assertEquals(
-        ImmutableList.copyOf(strings),
-        ImmutableList.copyOf(new QuotableWhiteSpaceSplitter(Joiner.on(" ").join(strings), mapper))
     );
   }
 }

indexing-service/src/test/java/io/druid/indexing/overlord/config/ForkingTaskRunnerConfigTest.java

@@ -0,0 +1,174 @@
+/*
+ * Licensed to Metamarkets Group Inc. (Metamarkets) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  Metamarkets 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 io.druid.indexing.overlord.config;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableList;
+import com.google.inject.Binder;
+import com.google.inject.Injector;
+import com.google.inject.Key;
+import com.google.inject.Module;
+import com.google.inject.name.Names;
+import io.druid.guice.GuiceInjectors;
+import io.druid.guice.IndexingServiceModuleHelper;
+import io.druid.guice.JsonConfigurator;
+import io.druid.initialization.Initialization;
+import io.druid.jackson.DefaultObjectMapper;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.List;
+import java.util.Properties;
+
+public class ForkingTaskRunnerConfigTest
+{
+  private static final ObjectMapper MAPPER = new DefaultObjectMapper();
+  private static final Injector INJECTOR = Initialization.makeInjectorWithModules(
+      GuiceInjectors.makeStartupInjector(),
+      ImmutableList.<Module>of(
+          new Module()
+          {
+            @Override
+            public void configure(Binder binder)
+            {
+              binder.bind(Key.get(String.class, Names.named("serviceName"))).toInstance("some service");
+              binder.bind(Key.get(Integer.class, Names.named("servicePort"))).toInstance(0);
+            }
+          }
+      )
+  );
+  private static final JsonConfigurator CONFIGURATOR = INJECTOR.getBinding(JsonConfigurator.class).getProvider().get();
+
+  @Test
+  public void testSimpleJavaOpts()
+  {
+    final ForkingTaskRunnerConfig forkingTaskRunnerConfig = CONFIGURATOR.configurate(
+        new Properties(),
+        "not found",
+        ForkingTaskRunnerConfig.class
+    );
+    Assert.assertEquals("", forkingTaskRunnerConfig.getJavaOpts());
+    Assert.assertEquals(ImmutableList.of(), forkingTaskRunnerConfig.getJavaOptsArray());
+  }
+
+  @Test
+  public void testSimpleStringJavaOpts()
+  {
+    final String javaOpts = "some string";
+    Assert.assertEquals(
+        javaOpts,
+        buildFromProperties(ForkingTaskRunnerConfig.JAVA_OPTS_PROPERTY, javaOpts).getJavaOpts()
+    );
+  }
+
+  @Test
+  public void testCrazyQuotesStringJavaOpts()
+  {
+    final String javaOpts = "            \"test\",\n"
+                            + "            \"-mmm\\\"some quote with\\\"suffix\",\n"
+                            + "            \"test2\",\n"
+                            + "            \"\\\"completely quoted\\\"\",\n"
+                            + "            \"more\",\n"
+                            + "            \"☃\",\n"
+                            + "            \"-XX:SomeCoolOption=false\",\n"
+                            + "            \"-XX:SomeOption=\\\"with spaces\\\"\",\n"
+                            + "            \"someValues\",\n"
+                            + "            \"some\\\"strange looking\\\"option\",\n"
+                            + "            \"andOtherOptions\",\n"
+                            + "            \"\\\"\\\"\",\n"
+                            + "            \"AndMaybeEmptyQuotes\",\n"
+                            + "            \"keep me around\"";
+    Assert.assertEquals(
+        javaOpts,
+        buildFromProperties(ForkingTaskRunnerConfig.JAVA_OPTS_PROPERTY, javaOpts).getJavaOpts()
+    );
+  }
+
+  @Test
+  public void testSimpleJavaOptArray() throws JsonProcessingException
+  {
+    final List<String> javaOpts = ImmutableList.of("option1", "option \"2\"");
+    Assert.assertEquals(
+        javaOpts,
+        buildFromProperties(
+            ForkingTaskRunnerConfig.JAVA_OPTS_ARRAY_PROPERTY,
+            MAPPER.writeValueAsString(javaOpts)
+        ).getJavaOptsArray()
+    );
+  }
+
+  @Test
+  public void testCrazyJavaOptArray() throws JsonProcessingException
+  {
+    final List<String> javaOpts = ImmutableList.of(
+        "test",
+        "-mmm\"some quote with\"suffix",
+        "test2",
+        "\"completely quoted\"",
+        "more",
+        "☃",
+        "-XX:SomeCoolOption=false",
+        "-XX:SomeOption=\"with spaces\"",
+        "someValues",
+        "some\"strange looking\"option",
+        "andOtherOptions",
+        "\"\"",
+        "AndMaybeEmptyQuotes",
+        "keep me around"
+    );
+    Assert.assertEquals(
+        javaOpts,
+        buildFromProperties(
+            ForkingTaskRunnerConfig.JAVA_OPTS_ARRAY_PROPERTY,
+            MAPPER.writeValueAsString(javaOpts)
+        ).getJavaOptsArray()
+    );
+  }
+
+  @Test(expected = com.google.inject.ProvisionException.class)
+  public void testExceptionalJavaOptArray() throws JsonProcessingException
+  {
+    buildFromProperties(ForkingTaskRunnerConfig.JAVA_OPTS_ARRAY_PROPERTY, "not an array");
+  }
+
+  @Test(expected = com.google.inject.ProvisionException.class)
+  public void testExceptionalJavaOpt() throws JsonProcessingException
+  {
+    buildFromProperties(ForkingTaskRunnerConfig.JAVA_OPTS_PROPERTY, "[\"not a string\"]");
+  }
+
+  @Test(expected = com.google.inject.ProvisionException.class)
+  public void testExceptionalJavaOpt2() throws JsonProcessingException
+  {
+    buildFromProperties(ForkingTaskRunnerConfig.JAVA_OPTS_PROPERTY, "{\"not a string\":\"someVal\"}");
+  }
+
+  private ForkingTaskRunnerConfig buildFromProperties(String key, String value)
+  {
+    final Properties properties = new Properties();
+    properties.put(key, value);
+    return CONFIGURATOR.configurate(
+        properties,
+        IndexingServiceModuleHelper.INDEXER_RUNNER_PROPERTY_PREFIX,
+        ForkingTaskRunnerConfig.class
+    );
+  }
+}