diff --git a/docs/content/configuration/indexing-service.md b/docs/content/configuration/indexing-service.md index 1cc758fb400..43cd484b44f 100644 --- a/docs/content/configuration/indexing-service.md +++ b/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| diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/ForkingTaskRunner.java b/indexing-service/src/main/java/io/druid/indexing/overlord/ForkingTaskRunner.java index 14f307b40fd..3a4f4806e8a 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/ForkingTaskRunner.java +++ b/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( - command, - new QuotableWhiteSpaceSplitter(config.getJavaOpts(), jsonMapper) - ); + Iterables.addAll(command, new QuotableWhiteSpaceSplitter(config.getJavaOpts())); + Iterables.addAll(command, config.getJavaOptsArray()); // 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 { 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 iterator() { - try (JsonParser parser = mapper.getFactory().createParser(string)) { - final JsonToken token = parser.nextToken(); - if (JsonToken.START_ARRAY.equals(token)) { - return mapper.>readValue(string, new TypeReference>() - { - }).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() { diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/config/ForkingTaskRunnerConfig.java b/indexing-service/src/main/java/io/druid/indexing/overlord/config/ForkingTaskRunnerConfig.java index d9d6ab4242c..7b3c6ba8bf3 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/config/ForkingTaskRunnerConfig.java +++ b/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 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 getJavaOptsArray() + { + return javaOptsArray; + } + public String getClasspath() { return classpath; diff --git a/indexing-service/src/test/java/io/druid/indexing/overlord/ForkingTaskRunnerTest.java b/indexing-service/src/test/java/io/druid/indexing/overlord/ForkingTaskRunnerTest.java index a000f81df0b..5f1fe0eb1be 100644 --- a/indexing-service/src/test/java/io/druid/indexing/overlord/ForkingTaskRunnerTest.java +++ b/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", - mapper + "start\t\t\t\t \n\f\r\n \f\f \n\r\f\n\r\t stop" ) ) ); @@ -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)) - ); - } - catch (JsonProcessingException e) { - throw Throwables.propagate(e); - } Assert.assertEquals( ImmutableList.copyOf(strings), - ImmutableList.copyOf(new QuotableWhiteSpaceSplitter(Joiner.on(" ").join(strings), mapper)) + ImmutableList.copyOf(new QuotableWhiteSpaceSplitter(Joiner.on(" ").join(strings))) ); } } diff --git a/indexing-service/src/test/java/io/druid/indexing/overlord/config/ForkingTaskRunnerConfigTest.java b/indexing-service/src/test/java/io/druid/indexing/overlord/config/ForkingTaskRunnerConfigTest.java new file mode 100644 index 00000000000..ba1b87df9e7 --- /dev/null +++ b/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.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 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 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 + ); + } +}