From 465035e53113c6452ea85480c33039ccf22a5774 Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Thu, 17 Sep 2015 13:35:03 -0700 Subject: [PATCH] Allow ForkingTaskRunner javaOpts to have quoted arguments which contain spaces --- .../indexing/overlord/ForkingTaskRunner.java | 54 ++++++-- .../overlord/ForkingTaskRunnerTest.java | 123 ++++++++++++++++++ 2 files changed, 168 insertions(+), 9 deletions(-) create mode 100644 indexing-service/src/test/java/io/druid/indexing/overlord/ForkingTaskRunnerTest.java 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 6d249f4284d..cf7279f87d0 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 @@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.CharMatcher; import com.google.common.base.Joiner; import com.google.common.base.Optional; +import com.google.common.base.Preconditions; import com.google.common.base.Splitter; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; @@ -55,6 +56,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.util.Collection; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Properties; @@ -69,8 +71,6 @@ public class ForkingTaskRunner implements TaskRunner, TaskLogStreamer { private static final EmittingLogger log = new EmittingLogger(ForkingTaskRunner.class); private static final String CHILD_PROPERTY_PREFIX = "druid.indexer.fork.property."; - private static final Splitter whiteSpaceSplitter = Splitter.on(CharMatcher.WHITESPACE).omitEmptyStrings(); - private final ForkingTaskRunnerConfig config; private final TaskConfig taskConfig; private final Properties props; @@ -172,18 +172,16 @@ public class ForkingTaskRunner implements TaskRunner, TaskLogStreamer command.add("-cp"); command.add(taskClasspath); - Iterables.addAll(command, whiteSpaceSplitter.split(config.getJavaOpts())); + Iterables.addAll(command, new QuotableWhiteSpaceSplitter(config.getJavaOpts())); // Override task specific javaOpts Object taskJavaOpts = task.getContextValue( "druid.indexer.runner.javaOpts" ); - if(taskJavaOpts != null) { + if (taskJavaOpts != null) { Iterables.addAll( command, - whiteSpaceSplitter.split( - (String) taskJavaOpts - ) + new QuotableWhiteSpaceSplitter((String) taskJavaOpts) ); } @@ -280,9 +278,11 @@ public class ForkingTaskRunner implements TaskRunner, TaskLogStreamer // Process exited unsuccessfully return TaskStatus.failure(task.getId()); } - } catch (Throwable t) { + } + catch (Throwable t) { throw closer.rethrow(t); - } finally { + } + finally { closer.close(); } } @@ -457,3 +457,39 @@ public class ForkingTaskRunner implements TaskRunner, TaskLogStreamer } } } + +/** + * Make an iterable of space delimited strings... unless there are quotes, which it preserves + */ +class QuotableWhiteSpaceSplitter implements Iterable +{ + private final String string; + + public QuotableWhiteSpaceSplitter(String string) + { + this.string = Preconditions.checkNotNull(string); + } + + @Override + public Iterator iterator() + { + return Splitter.on( + new CharMatcher() + { + private boolean inQuotes = false; + + @Override + public boolean matches(char c) + { + if ('"' == c) { + inQuotes = !inQuotes; + } + if (inQuotes) { + return false; + } + return CharMatcher.BREAKING_WHITESPACE.matches(c); + } + } + ).omitEmptyStrings().split(string).iterator(); + } +} 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 new file mode 100644 index 00000000000..f40f5446cbf --- /dev/null +++ b/indexing-service/src/test/java/io/druid/indexing/overlord/ForkingTaskRunnerTest.java @@ -0,0 +1,123 @@ +/* + * 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; + +import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterators; +import org.junit.Assert; +import org.junit.Test; + +public class ForkingTaskRunnerTest +{ + // This tests the test to make sure the test fails when it should. + @Test(expected = AssertionError.class) + public void testPatternMatcherFailureForJavaOptions() + { + checkValues(new String[]{"not quoted has space"}); + } + + @Test(expected = AssertionError.class) + public void testPatternMatcherFailureForSpaceOnlyJavaOptions() + { + checkValues(new String[]{" "}); + } + + @Test + public void testPatternMatcherLeavesUnbalancedQuoteJavaOptions() + { + Assert.assertEquals("\"", Iterators.get(new QuotableWhiteSpaceSplitter("\"").iterator(), 0)); + } + + @Test + public void testPatternMatcherPreservesNonBreakingSpacesJavaOptions() + { + // SUPER AWESOME VOODOO MAGIC. These are not normal spaces, they are non-breaking spaces. + checkValues(new String[]{"keep me around"}); + } + + + @Test + public void testPatternMatcherForSimpleJavaOptions() + { + checkValues( + new String[]{ + "test", + "-mmm\"some quote with\"suffix", + "test2", + "\"completely quoted\"", + "more", + "☃", + "-XX:SomeCoolOption=false", + "-XX:SomeOption=\"with spaces\"", + "someValues", + "some\"strange looking\"option", + "andOtherOptions", + "\"\"", + "AndMaybeEmptyQuotes" + } + ); + checkValues(new String[]{"\"completely quoted\""}); + checkValues(new String[]{"\"\""}); + checkValues(new String[]{"-foo=\"\""}); + checkValues(new String[]{"-foo=\"\"suffix"}); + checkValues(new String[]{"-foo=\"\t\"suffix"}); + checkValues(new String[]{"-foo=\"\t\r\n\f \"suffix"}); + checkValues(new String[]{"-foo=\"\t\r\n\f \""}); + checkValues(new String[]{"\"\t\r\n\f \"suffix"}); + checkValues(new String[]{"-foo=\"\"suffix", "more"}); + } + + @Test + public void testEmpty() + { + Assert.assertTrue(ImmutableList.copyOf(new QuotableWhiteSpaceSplitter("")).isEmpty()); + } + + @Test + public void testFarApart() + { + 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" + ) + ) + ); + } + + @Test + public void testOmitEmpty() + { + Assert.assertTrue( + ImmutableList.copyOf( + new QuotableWhiteSpaceSplitter(" \t \t\t\t\t \n\n \f\f \n\f\r\t") + ).isEmpty() + ); + } + + private static void checkValues(String[] strings) + { + Assert.assertEquals( + ImmutableList.copyOf(strings), + ImmutableList.copyOf(new QuotableWhiteSpaceSplitter(Joiner.on(" ").join(strings))) + ); + } +}