From 8c17315f3a6920971a497ffc9a76c74ec636a96c Mon Sep 17 00:00:00 2001 From: Timothy Farkas Date: Thu, 21 Sep 2017 22:40:46 -0700 Subject: [PATCH 1/7] DRILL-5809 made the storage format of system options backwards compatible, and avoided storing unnecessary option info. --- .../drill/testutils/DirTestWatcher.java | 62 +++++++++ .../server/options/BaseOptionManager.java | 2 +- .../exec/server/options/OptionMetaData.java | 2 +- .../exec/server/options/OptionValue.java | 60 ++++++--- .../server/options/PersistedOptionValue.java | 125 ++++++++++++++++++ .../server/options/SystemOptionManager.java | 34 ++--- .../java/org/apache/drill/BaseTestQuery.java | 14 +- .../TestInboundImpersonationPrivileges.java | 2 +- .../exec/server/options/OptionValueTest.java | 72 ++++++++++ .../options/PersistedOptionValueTest.java | 69 ++++++++++ .../exec/store/sys/TestPStoreProviders.java | 123 +++++++++++++++-- .../resources/options/new_booleanopt.json | 3 + .../test/resources/options/new_doubleopt.json | 3 + .../test/resources/options/new_longopt.json | 3 + .../test/resources/options/new_stringopt.json | 3 + .../resources/options/old_booleanopt.json | 6 + .../test/resources/options/old_doubleopt.json | 6 + .../test/resources/options/old_longopt.json | 6 + .../test/resources/options/old_stringopt.json | 6 + ...wgroup.filter.pushdown.threshold.sys.drill | 3 + .../planner.width.max_per_query.sys.drill | 3 + ...wgroup.filter.pushdown.threshold.sys.drill | 7 + .../planner.width.max_per_query.sys.drill | 7 + pom.xml | 1 + 24 files changed, 578 insertions(+), 44 deletions(-) create mode 100644 common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java create mode 100644 exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java create mode 100644 exec/java-exec/src/test/java/org/apache/drill/exec/server/options/OptionValueTest.java create mode 100644 exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java create mode 100644 exec/java-exec/src/test/resources/options/new_booleanopt.json create mode 100644 exec/java-exec/src/test/resources/options/new_doubleopt.json create mode 100644 exec/java-exec/src/test/resources/options/new_longopt.json create mode 100644 exec/java-exec/src/test/resources/options/new_stringopt.json create mode 100644 exec/java-exec/src/test/resources/options/old_booleanopt.json create mode 100644 exec/java-exec/src/test/resources/options/old_doubleopt.json create mode 100644 exec/java-exec/src/test/resources/options/old_longopt.json create mode 100644 exec/java-exec/src/test/resources/options/old_stringopt.json create mode 100644 exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill create mode 100644 exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.width.max_per_query.sys.drill create mode 100644 exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill create mode 100644 exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.width.max_per_query.sys.drill diff --git a/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java b/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java new file mode 100644 index 00000000000..cad822d913c --- /dev/null +++ b/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java @@ -0,0 +1,62 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.drill.testutils; + +import org.apache.commons.io.FileUtils; +import org.junit.rules.TestWatcher; +import org.junit.runner.Description; + +import java.io.File; + +/** + * This JUnit {@link TestWatcher} creates a unique directory for each JUnit test in the project's + * target folder at the start of each test. This directory can be used as a temporary directory to store + * files for the test. The directory and its contents are deleted at the end of the test. + */ + +public class DirTestWatcher extends TestWatcher { + private String dirPath; + private File dir; + + @Override + protected void starting(Description description) { + dirPath = String.format("%s/target/%s/%s", new File(".").getAbsolutePath(), + description.getClassName(), description.getMethodName()); + dir = new File(dirPath); + dir.mkdirs(); + } + + @Override + protected void finished(Description description) { + FileUtils.deleteQuietly(dir); + } + + @Override + protected void failed(Throwable e, Description description) { + FileUtils.deleteQuietly(dir); + } + + public String getDirPath() { + return dirPath; + } + + public File getDir() { + return dir; + } +} \ No newline at end of file diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/BaseOptionManager.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/BaseOptionManager.java index 26f9108c623..5a0fa2d91c7 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/BaseOptionManager.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/BaseOptionManager.java @@ -62,7 +62,7 @@ public void setLocalOption(final String name, final Object value) { final OptionDefinition definition = getOptionDefinition(name); final OptionValidator validator = definition.getValidator(); final OptionMetaData metaData = definition.getMetaData(); - final OptionValue.AccessibleScopes type = definition.getMetaData().getType(); + final OptionValue.AccessibleScopes type = definition.getMetaData().getAccessibleScopes(); final OptionValue.OptionScope scope = getScope(); checkOptionPermissions(name, type, scope); final OptionValue optionValue = OptionValue.create(type, name, value, scope); diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionMetaData.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionMetaData.java index f73c3486148..6d2762fb598 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionMetaData.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionMetaData.java @@ -34,7 +34,7 @@ public OptionMetaData(OptionValue.AccessibleScopes type, boolean adminOnly, bool this.internal = internal; } - public OptionValue.AccessibleScopes getType() { + public OptionValue.AccessibleScopes getAccessibleScopes() { return type; } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java index 0a49f5d4a5d..76d82c8c20c 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java @@ -44,6 +44,14 @@ */ @JsonInclude(Include.NON_NULL) public class OptionValue implements Comparable { + public static final String JSON_KIND = "kind"; + public static final String JSON_ACCESSIBLE_SCOPES = "accessibleScopes"; + public static final String JSON_NAME = "name"; + public static final String JSON_NUM_VAL = "num_val"; + public static final String JSON_STRING_VAL = "string_val"; + public static final String JSON_BOOL_VAL = "bool_val"; + public static final String JSON_FLOAT_VAL = "float_val"; + public static final String JSON_SCOPE = "scope"; /** * Defines where an option can be configured. @@ -88,20 +96,36 @@ public enum OptionScope { public final Double float_val; public final OptionScope scope; - public static OptionValue create(AccessibleScopes type, String name, long val, OptionScope scope) { - return new OptionValue(Kind.LONG, type, name, val, null, null, null, scope); + public static OptionValue create(AccessibleScopes accessibleScopes, String name, long val, OptionScope scope) { + return new OptionValue(Kind.LONG, accessibleScopes, name, val, null, null, null, scope); } - public static OptionValue create(AccessibleScopes type, String name, boolean bool, OptionScope scope) { - return new OptionValue(Kind.BOOLEAN, type, name, null, null, bool, null, scope); + public static OptionValue create(AccessibleScopes accessibleScopes, String name, boolean bool, OptionScope scope) { + return new OptionValue(Kind.BOOLEAN, accessibleScopes, name, null, null, bool, null, scope); } - public static OptionValue create(AccessibleScopes type, String name, String val, OptionScope scope) { - return new OptionValue(Kind.STRING, type, name, null, val, null, null, scope); + public static OptionValue create(AccessibleScopes accessibleScopes, String name, String val, OptionScope scope) { + return new OptionValue(Kind.STRING, accessibleScopes, name, null, val, null, null, scope); } - public static OptionValue create(AccessibleScopes type, String name, double val, OptionScope scope) { - return new OptionValue(Kind.DOUBLE, type, name, null, null, null, val, scope); + public static OptionValue create(AccessibleScopes accessibleScopes, String name, double val, OptionScope scope) { + return new OptionValue(Kind.DOUBLE, accessibleScopes, name, null, null, null, val, scope); + } + + public static OptionValue create(Kind kind, AccessibleScopes accessibleScopes, + String name, String val, OptionScope scope) { + switch (kind) { + case BOOLEAN: + return create(accessibleScopes, name, Boolean.valueOf(val), scope); + case STRING: + return create(accessibleScopes, name, val, scope); + case DOUBLE: + return create(accessibleScopes, name, Double.parseDouble(val), scope); + case LONG: + return create(accessibleScopes, name, Long.parseLong(val), scope); + default: + throw new IllegalArgumentException(String.format("Unsupported kind %s", kind)); + } } public static OptionValue create(AccessibleScopes type, String name, Object val, OptionScope scope) { @@ -119,14 +143,14 @@ public static OptionValue create(AccessibleScopes type, String name, Object val, } @JsonCreator - private OptionValue(@JsonProperty("kind") Kind kind, - @JsonProperty("accessibleScopes") AccessibleScopes accessibleScopes, - @JsonProperty("name") String name, - @JsonProperty("num_val") Long num_val, - @JsonProperty("string_val") String string_val, - @JsonProperty("bool_val") Boolean bool_val, - @JsonProperty("float_val") Double float_val, - @JsonProperty("scope") OptionScope scope) { + private OptionValue(@JsonProperty(JSON_KIND) Kind kind, + @JsonProperty(JSON_ACCESSIBLE_SCOPES) AccessibleScopes accessibleScopes, + @JsonProperty(JSON_NAME) String name, + @JsonProperty(JSON_NUM_VAL) Long num_val, + @JsonProperty(JSON_STRING_VAL) String string_val, + @JsonProperty(JSON_BOOL_VAL) Boolean bool_val, + @JsonProperty(JSON_FLOAT_VAL) Double float_val, + @JsonProperty(JSON_SCOPE) OptionScope scope) { Preconditions.checkArgument(num_val != null || string_val != null || bool_val != null || float_val != null); this.kind = kind; this.accessibleScopes = accessibleScopes; @@ -158,6 +182,10 @@ public Object getValue() { } } + public PersistedOptionValue toPersisted() { + return new PersistedOptionValue(getValue().toString()); + } + @Override public int hashCode() { final int prime = 31; diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java new file mode 100644 index 00000000000..c65b681abee --- /dev/null +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java @@ -0,0 +1,125 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.drill.exec.server.options; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.ObjectCodec; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JavaType; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.annotation.JsonDeserialize; +import com.fasterxml.jackson.databind.deser.std.StdDeserializer; +import com.google.common.base.Preconditions; + +import java.io.IOException; + +// Custom Deserializer required for backward compatibility DRILL-5809 +@JsonDeserialize(using = PersistedOptionValue.Deserializer.class) +public class PersistedOptionValue { + public static final String JSON_VALUE = "value"; + + private String value; + + public PersistedOptionValue(@JsonProperty(JSON_VALUE) String value) { + this.value = Preconditions.checkNotNull(value); + } + + public String getValue() { + return value; + } + + public OptionValue toOptionValue(final OptionDefinition optionDefinition, final OptionValue.OptionScope optionScope) { + final OptionValidator validator = optionDefinition.getValidator(); + final OptionValue.Kind kind = validator.getKind(); + final String name = validator.getOptionName(); + final OptionValue.AccessibleScopes accessibleScopes = optionDefinition.getMetaData().getAccessibleScopes(); + + return OptionValue.create(kind, accessibleScopes, name, value, optionScope); + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + + if (o == null || getClass() != o.getClass()) { + return false; + } + + PersistedOptionValue that = (PersistedOptionValue) o; + + return value.equals(that.value); + } + + @Override + public int hashCode() { + return value.hashCode(); + } + + @Override + public String toString() { + return "PersistedOptionValue{" + "value='" + value + '\'' + '}'; + } + + public static class Deserializer extends StdDeserializer { + private Deserializer() { + super(PersistedOptionValue.class); + } + + protected Deserializer(JavaType valueType) { + super(valueType); + } + + protected Deserializer(StdDeserializer src) { + super(src); + } + + @Override + public PersistedOptionValue deserialize(JsonParser p, DeserializationContext ctxt) throws IOException, JsonProcessingException { + ObjectCodec oc = p.getCodec(); + JsonNode node = oc.readTree(p); + String value = null; + + if (node.has(OptionValue.JSON_NUM_VAL)) { + value = node.get(OptionValue.JSON_NUM_VAL).asText(); + } + + if (node.has(OptionValue.JSON_STRING_VAL)) { + value = node.get(OptionValue.JSON_STRING_VAL).asText(); + } + + if (node.has(OptionValue.JSON_BOOL_VAL)) { + value = node.get(OptionValue.JSON_BOOL_VAL).asText(); + } + + if (node.has(OptionValue.JSON_FLOAT_VAL)) { + value = node.get(OptionValue.JSON_FLOAT_VAL).asText(); + } + + if (node.has(JSON_VALUE)) { + value = node.get(JSON_VALUE).asText(); + } + + return new PersistedOptionValue(value); + } + } +} diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java index 75bcc1f8148..ba963ee3b90 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java @@ -17,8 +17,6 @@ */ package org.apache.drill.exec.server.options; -import static com.google.common.base.Preconditions.checkArgument; - import java.io.IOException; import java.util.HashMap; import java.util.Iterator; @@ -217,7 +215,7 @@ public static CaseInsensitiveMap createDefaultOptionDefinition return map; } - private final PersistentStoreConfig config; + private final PersistentStoreConfig config; private final PersistentStoreProvider provider; @@ -226,7 +224,7 @@ public static CaseInsensitiveMap createDefaultOptionDefinition * Persistent store for options that have been changed from default. * NOTE: CRUD operations must use lowercase keys. */ - private PersistentStore options; + private PersistentStore options; private CaseInsensitiveMap definitions; private CaseInsensitiveMap defaults; @@ -238,7 +236,7 @@ public SystemOptionManager(LogicalPlanPersistence lpPersistence, final Persisten public SystemOptionManager(final LogicalPlanPersistence lpPersistence, final PersistentStoreProvider provider, final DrillConfig bootConfig, final CaseInsensitiveMap definitions) { this.provider = provider; - this.config = PersistentStoreConfig.newJacksonBuilder(lpPersistence.getMapper(), OptionValue.class) + this.config = PersistentStoreConfig.newJacksonBuilder(lpPersistence.getMapper(), PersistedOptionValue.class) .name("sys.options") .build(); this.bootConfig = bootConfig; @@ -255,7 +253,7 @@ public SystemOptionManager(final LogicalPlanPersistence lpPersistence, final Per public SystemOptionManager init() throws Exception { options = provider.getOrCreateStore(config); // if necessary, deprecate and replace options from persistent store - for (final Entry option : Lists.newArrayList(options.getAll())) { + for (final Entry option : Lists.newArrayList(options.getAll())) { final String name = option.getKey(); final OptionDefinition definition = definitions.get(name); if (definition == null) { @@ -268,7 +266,7 @@ public SystemOptionManager init() throws Exception { if (!name.equals(canonicalName)) { // for backwards compatibility <= 1.1, rename to lower case. logger.warn("Changing option name to lower case `{}`", name); - final OptionValue value = option.getValue(); + final PersistedOptionValue value = option.getValue(); options.delete(name); options.put(canonicalName, value); } @@ -286,8 +284,13 @@ public Iterator iterator() { buildList.put(entry.getKey(), entry.getValue()); } // override if changed - for (final Map.Entry entry : Lists.newArrayList(options.getAll())) { - buildList.put(entry.getKey(), entry.getValue()); + for (final Map.Entry entry : Lists.newArrayList(options.getAll())) { + final String name = entry.getKey(); + final OptionDefinition optionDefinition = getOptionDefinition(name); + final PersistedOptionValue persistedOptionValue = entry.getValue(); + final OptionValue optionValue = persistedOptionValue + .toOptionValue(optionDefinition, OptionValue.OptionScope.SYSTEM); + buildList.put(name, optionValue); } return buildList.values().iterator(); } @@ -295,10 +298,11 @@ public Iterator iterator() { @Override public OptionValue getOption(final String name) { // check local space (persistent store) - final OptionValue value = options.get(name.toLowerCase()); + final PersistedOptionValue persistedValue = options.get(name.toLowerCase()); - if (value != null) { - return value; + if (persistedValue != null) { + final OptionDefinition optionDefinition = getOptionDefinition(name); + return persistedValue.toOptionValue(optionDefinition, OptionValue.OptionScope.SYSTEM); } // otherwise, return default set in the validator. @@ -322,7 +326,7 @@ protected void setLocalOptionHelper(final OptionValue value) { return; // if the option is not overridden, ignore setting option to default } - options.put(name, value); + options.put(name, value.toPersisted()); } @Override @@ -339,7 +343,7 @@ public void deleteLocalOption(final String name) { @Override public void deleteAllLocalOptions() { final Set names = Sets.newHashSet(); - for (final Map.Entry entry : Lists.newArrayList(options.getAll())) { + for (final Map.Entry entry : Lists.newArrayList(options.getAll())) { names.add(entry.getKey()); } for (final String name : names) { @@ -354,7 +358,7 @@ public static CaseInsensitiveMap populateDefaultValues(Map entry : definitions.entrySet()) { final OptionDefinition definition = entry.getValue(); final OptionMetaData metaData = definition.getMetaData(); - final OptionValue.AccessibleScopes type = metaData.getType(); + final OptionValue.AccessibleScopes type = metaData.getAccessibleScopes(); final OptionValidator validator = definition.getValidator(); final String name = validator.getOptionName(); final String configName = validator.getConfigProperty(); diff --git a/exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java b/exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java index e30c5e9e8b7..d40233d5d25 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java +++ b/exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java @@ -92,6 +92,8 @@ public class BaseTestQuery extends ExecTest { { put(ExecConstants.SYS_STORE_PROVIDER_LOCAL_ENABLE_WRITE, "false"); put(ExecConstants.HTTP_ENABLE, "false"); + // Increasing retry attempts for testing + put(ExecConstants.UDF_RETRY_ATTEMPTS, "10"); } }; @@ -198,6 +200,10 @@ private static void openClient() throws Exception { } private static void openClient(Properties properties) throws Exception { + if (properties == null) { + properties = new Properties(); + } + allocator = RootAllocatorFactory.newRoot(config); serviceSet = RemoteServiceSet.getLocalServiceSet(); @@ -213,7 +219,13 @@ private static void openClient(Properties properties) throws Exception { TestUtilities.makeDfsTmpSchemaImmutable(pluginRegistry); } - client = QueryTestUtil.createClient(config, serviceSet, MAX_WIDTH_PER_NODE, properties); + if (!properties.containsKey(DrillProperties.DRILLBIT_CONNECTION)) { + properties = new Properties(properties); + properties.setProperty(DrillProperties.DRILLBIT_CONNECTION, + String.format("localhost:%s", bits[0].getUserPort())); + } + + client = QueryTestUtil.createClient(config, serviceSet, MAX_WIDTH_PER_NODE, properties); } /** diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/impersonation/TestInboundImpersonationPrivileges.java b/exec/java-exec/src/test/java/org/apache/drill/exec/impersonation/TestInboundImpersonationPrivileges.java index f452669177c..1c656e00db4 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/impersonation/TestInboundImpersonationPrivileges.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/impersonation/TestInboundImpersonationPrivileges.java @@ -50,7 +50,7 @@ public class TestInboundImpersonationPrivileges extends BaseTestImpersonation { private static boolean checkPrivileges(final String proxyName, final String targetName) { OptionDefinition optionDefinition = SystemOptionManager.createDefaultOptionDefinitions().get(ExecConstants.IMPERSONATION_POLICIES_KEY); ExecConstants.IMPERSONATION_POLICY_VALIDATOR.validate( - OptionValue.create(optionDefinition.getMetaData().getType(), + OptionValue.create(optionDefinition.getMetaData().getAccessibleScopes(), ExecConstants.IMPERSONATION_POLICIES_KEY, IMPERSONATION_POLICIES,OptionValue.OptionScope.SYSTEM), optionDefinition.getMetaData(),null); try { diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/OptionValueTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/OptionValueTest.java new file mode 100644 index 00000000000..e590c506b4b --- /dev/null +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/OptionValueTest.java @@ -0,0 +1,72 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.drill.exec.server.options; + +import org.junit.Assert; +import org.junit.Test; + +public class OptionValueTest { + @Test + public void createBooleanKindTest() { + final OptionValue createdValue = OptionValue.create( + OptionValue.Kind.BOOLEAN, OptionValue.AccessibleScopes.ALL, + "myOption", "true", OptionValue.OptionScope.SYSTEM); + + final OptionValue expectedValue = OptionValue.create( + OptionValue.AccessibleScopes.ALL, "myOption", true, OptionValue.OptionScope.SYSTEM); + + Assert.assertEquals(expectedValue, createdValue); + } + + @Test + public void createDoubleKindTest() { + final OptionValue createdValue = OptionValue.create( + OptionValue.Kind.DOUBLE, OptionValue.AccessibleScopes.ALL, + "myOption", "1.5", OptionValue.OptionScope.SYSTEM); + + final OptionValue expectedValue = OptionValue.create( + OptionValue.AccessibleScopes.ALL, "myOption", 1.5, OptionValue.OptionScope.SYSTEM); + + Assert.assertEquals(expectedValue, createdValue); + } + + @Test + public void createLongKindTest() { + final OptionValue createdValue = OptionValue.create( + OptionValue.Kind.LONG, OptionValue.AccessibleScopes.ALL, + "myOption", "3000", OptionValue.OptionScope.SYSTEM); + + final OptionValue expectedValue = OptionValue.create( + OptionValue.AccessibleScopes.ALL, "myOption", 3000, OptionValue.OptionScope.SYSTEM); + + Assert.assertEquals(expectedValue, createdValue); + } + + @Test + public void createStringKindTest() { + final OptionValue createdValue = OptionValue.create( + OptionValue.Kind.STRING, OptionValue.AccessibleScopes.ALL, + "myOption", "wabalubawubdub", OptionValue.OptionScope.SYSTEM); + + final OptionValue expectedValue = OptionValue.create( + OptionValue.AccessibleScopes.ALL, "myOption", "wabalubawubdub", OptionValue.OptionScope.SYSTEM); + + Assert.assertEquals(expectedValue, createdValue); + } +} diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java new file mode 100644 index 00000000000..bbfa8cd10d1 --- /dev/null +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java @@ -0,0 +1,69 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.drill.exec.server.options; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.drill.common.util.FileUtils; +import org.apache.drill.exec.serialization.JacksonSerializer; +import org.junit.Assert; +import org.junit.Test; + +import java.io.IOException; + +public class PersistedOptionValueTest { + @Test + public void oldDeserializeTest() throws IOException { + testHelper("/options/old_booleanopt.json", + "/options/old_doubleopt.json", + "/options/old_longopt.json", + "/options/old_stringopt.json"); + } + + @Test + public void newDeserializeTest() throws IOException { + testHelper("/options/new_booleanopt.json", + "/options/new_doubleopt.json", + "/options/new_longopt.json", + "/options/new_stringopt.json"); + } + + private void testHelper(String booleanOptionFile, String doubleOptionFile, + String longOptionFile, String stringOptionFile) throws IOException { + JacksonSerializer serializer = new JacksonSerializer<>(new ObjectMapper(), PersistedOptionValue.class); + String booleanOptionJson = FileUtils.getResourceAsString(booleanOptionFile); + String doubleOptionJson = FileUtils.getResourceAsString(doubleOptionFile); + String longOptionJson = FileUtils.getResourceAsString(longOptionFile); + String stringOptionJson = FileUtils.getResourceAsString(stringOptionFile); + + PersistedOptionValue booleanValue = (PersistedOptionValue) serializer.deserialize(booleanOptionJson.getBytes()); + PersistedOptionValue doubleValue = (PersistedOptionValue) serializer.deserialize(doubleOptionJson.getBytes()); + PersistedOptionValue longValue = (PersistedOptionValue) serializer.deserialize(longOptionJson.getBytes()); + PersistedOptionValue stringValue = (PersistedOptionValue) serializer.deserialize(stringOptionJson.getBytes()); + + PersistedOptionValue expectedBooleanValue = new PersistedOptionValue("true"); + PersistedOptionValue expectedDoubleValue = new PersistedOptionValue("1.5"); + PersistedOptionValue expectedLongValue = new PersistedOptionValue("5000"); + PersistedOptionValue expectedStringValue = new PersistedOptionValue("wabalubadubdub"); + + Assert.assertEquals(expectedBooleanValue, booleanValue); + Assert.assertEquals(expectedDoubleValue, doubleValue); + Assert.assertEquals(expectedLongValue, longValue); + Assert.assertEquals(expectedStringValue, stringValue); + } +} diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/store/sys/TestPStoreProviders.java b/exec/java-exec/src/test/java/org/apache/drill/exec/store/sys/TestPStoreProviders.java index 0504bb7cc99..91600d6752b 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/store/sys/TestPStoreProviders.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/store/sys/TestPStoreProviders.java @@ -15,21 +15,40 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + package org.apache.drill.exec.store.sys; +import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.curator.framework.CuratorFramework; import org.apache.curator.framework.CuratorFrameworkFactory; import org.apache.curator.retry.RetryNTimes; import org.apache.drill.common.config.DrillConfig; +import org.apache.drill.common.util.FileUtils; import org.apache.drill.exec.ExecConstants; import org.apache.drill.exec.TestWithZookeeper; +import org.apache.drill.exec.coord.zk.PathUtils; +import org.apache.drill.exec.coord.zk.ZookeeperClient; +import org.apache.drill.exec.planner.physical.PlannerSettings; +import org.apache.drill.exec.server.options.PersistedOptionValue; import org.apache.drill.exec.store.sys.store.provider.LocalPersistentStoreProvider; import org.apache.drill.exec.store.sys.store.provider.ZookeeperPersistentStoreProvider; +import org.apache.drill.test.ClientFixture; +import org.apache.drill.test.ClusterFixture; +import org.apache.drill.test.FixtureBuilder; +import org.apache.drill.testutils.DirTestWatcher; +import org.apache.zookeeper.CreateMode; +import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import java.io.File; + public class TestPStoreProviders extends TestWithZookeeper { static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TestPStoreProviders.class); + @Rule + public DirTestWatcher dirTestWatcher = new DirTestWatcher(); + @Test public void verifyLocalStore() throws Exception { try(LocalPersistentStoreProvider provider = new LocalPersistentStoreProvider(DrillConfig.create())){ @@ -39,19 +58,105 @@ public void verifyLocalStore() throws Exception { @Test public void verifyZkStore() throws Exception { + try(CuratorFramework curator = createCurator()){ + curator.start(); + ZookeeperPersistentStoreProvider provider = new ZookeeperPersistentStoreProvider(zkHelper.getConfig(), curator); + PStoreTestUtil.test(provider); + } + } + + @Test + public void localLoadTest() throws Exception { + localLoadTestHelper("src/test/resources/options/store/local/new/sys.options"); + } + + // DRILL-5809 + @Test + public void localBackwardCompatabilityTest() throws Exception { + localLoadTestHelper("src/test/resources/options/store/local/old/sys.options"); + } + + private void localLoadTestHelper(String propertiesDir) throws Exception { + File localOptionsResources = new File(propertiesDir); + + String optionsDirPath = String.format("%s/sys.options", dirTestWatcher.getDirPath()); + File optionsDir = new File(optionsDirPath); + optionsDir.mkdirs(); + + org.apache.commons.io.FileUtils.copyDirectory(localOptionsResources, optionsDir); + + FixtureBuilder builder = ClusterFixture.builder(). + configProperty(ExecConstants.HTTP_ENABLE, false). + configProperty(ExecConstants.SYS_STORE_PROVIDER_CLASS, LocalPersistentStoreProvider.class.getCanonicalName()). + configProperty(ExecConstants.SYS_STORE_PROVIDER_LOCAL_PATH, String.format("file://%s", dirTestWatcher.getDirPath())). + configProperty(ExecConstants.SYS_STORE_PROVIDER_LOCAL_ENABLE_WRITE, true); + + try (ClusterFixture cluster = builder.build(); + ClientFixture client = cluster.clientFixture()) { + String parquetPushdown = client.queryBuilder(). + sql("SELECT val FROM sys.%s where name='%s'", + SystemTable.OPTION_VAL.getTableName(), + PlannerSettings.PARQUET_ROWGROUP_FILTER_PUSHDOWN_PLANNING_THRESHOLD_KEY). + singletonString(); + + String plannerWidth = client.queryBuilder(). + sql("SELECT val FROM sys.%s where name='%s'", + SystemTable.OPTION_VAL.getTableName(), + ExecConstants.MAX_WIDTH_GLOBAL_KEY). + singletonString(); + + Assert.assertEquals("30000", parquetPushdown); + Assert.assertEquals("3333", plannerWidth); + } + } + + // DRILL-5809 + @Test + public void zkBackwardCompatabilityTest() throws Exception { + final String oldName = "myOldOption"; + final String newName = "myNewOption"; + + try (CuratorFramework curator = createCurator()) { + curator.start(); + + PersistentStoreConfig storeConfig = + PersistentStoreConfig.newJacksonBuilder(new ObjectMapper(), PersistedOptionValue.class).name("sys.test").build(); + + try (ZookeeperClient zkClient = new ZookeeperClient(curator, + PathUtils.join("/", storeConfig.getName()), CreateMode.PERSISTENT)) { + zkClient.start(); + String oldOptionJson = FileUtils.getResourceAsString("/options/old_booleanopt.json"); + String newOptionJson = FileUtils.getResourceAsString("/options/new_booleanopt.json"); + zkClient.put(oldName, oldOptionJson.getBytes(), null); + zkClient.put(newName, newOptionJson.getBytes(), null); + } + + try (ZookeeperPersistentStoreProvider provider = + new ZookeeperPersistentStoreProvider(zkHelper.getConfig(), curator)) { + PersistentStore store = provider.getOrCreateStore(storeConfig); + + PersistedOptionValue oldOptionValue = store.get(oldName, null); + PersistedOptionValue newOptionValue = store.get(newName, null); + + PersistedOptionValue expectedOld = new PersistedOptionValue("true"); + PersistedOptionValue expectedNew = new PersistedOptionValue("true"); + + Assert.assertEquals(expectedOld, oldOptionValue); + Assert.assertEquals(expectedNew, newOptionValue); + } + } + } + + private CuratorFramework createCurator() { String connect = zkHelper.getConnectionString(); DrillConfig config = zkHelper.getConfig(); CuratorFrameworkFactory.Builder builder = CuratorFrameworkFactory.builder() - .namespace(zkHelper.getConfig().getString(ExecConstants.ZK_ROOT)) - .retryPolicy(new RetryNTimes(1, 100)) - .connectionTimeoutMs(config.getInt(ExecConstants.ZK_TIMEOUT)) - .connectString(connect); + .namespace(zkHelper.getConfig().getString(ExecConstants.ZK_ROOT)) + .retryPolicy(new RetryNTimes(1, 100)) + .connectionTimeoutMs(config.getInt(ExecConstants.ZK_TIMEOUT)) + .connectString(connect); - try(CuratorFramework curator = builder.build()){ - curator.start(); - ZookeeperPersistentStoreProvider provider = new ZookeeperPersistentStoreProvider(config, curator); - PStoreTestUtil.test(provider); - } + return builder.build(); } } diff --git a/exec/java-exec/src/test/resources/options/new_booleanopt.json b/exec/java-exec/src/test/resources/options/new_booleanopt.json new file mode 100644 index 00000000000..302d1fd3b6f --- /dev/null +++ b/exec/java-exec/src/test/resources/options/new_booleanopt.json @@ -0,0 +1,3 @@ +{ + "value": "true" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/new_doubleopt.json b/exec/java-exec/src/test/resources/options/new_doubleopt.json new file mode 100644 index 00000000000..d6f89532804 --- /dev/null +++ b/exec/java-exec/src/test/resources/options/new_doubleopt.json @@ -0,0 +1,3 @@ +{ + "value": "1.5" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/new_longopt.json b/exec/java-exec/src/test/resources/options/new_longopt.json new file mode 100644 index 00000000000..186deac0f20 --- /dev/null +++ b/exec/java-exec/src/test/resources/options/new_longopt.json @@ -0,0 +1,3 @@ +{ + "value": "5000" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/new_stringopt.json b/exec/java-exec/src/test/resources/options/new_stringopt.json new file mode 100644 index 00000000000..64153096924 --- /dev/null +++ b/exec/java-exec/src/test/resources/options/new_stringopt.json @@ -0,0 +1,3 @@ +{ + "value": "wabalubadubdub" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/old_booleanopt.json b/exec/java-exec/src/test/resources/options/old_booleanopt.json new file mode 100644 index 00000000000..5614f5f1710 --- /dev/null +++ b/exec/java-exec/src/test/resources/options/old_booleanopt.json @@ -0,0 +1,6 @@ +{ + "kind": "BOOLEAN", + "name": "myOldOption", + "bool_val": true, + "type": "SYSTEM" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/old_doubleopt.json b/exec/java-exec/src/test/resources/options/old_doubleopt.json new file mode 100644 index 00000000000..08d39248580 --- /dev/null +++ b/exec/java-exec/src/test/resources/options/old_doubleopt.json @@ -0,0 +1,6 @@ +{ + "kind": "DOUBLE", + "name": "myOldOption", + "float_val": 1.5, + "type": "SYSTEM" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/old_longopt.json b/exec/java-exec/src/test/resources/options/old_longopt.json new file mode 100644 index 00000000000..cd551731bca --- /dev/null +++ b/exec/java-exec/src/test/resources/options/old_longopt.json @@ -0,0 +1,6 @@ +{ + "kind": "LONG", + "name": "myOldOption", + "num_val": 5000, + "type": "SYSTEM" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/old_stringopt.json b/exec/java-exec/src/test/resources/options/old_stringopt.json new file mode 100644 index 00000000000..c4fe69960b9 --- /dev/null +++ b/exec/java-exec/src/test/resources/options/old_stringopt.json @@ -0,0 +1,6 @@ +{ + "kind": "STRING", + "name": "myOldOption", + "string_val": "wabalubadubdub", + "type": "SYSTEM" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill b/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill new file mode 100644 index 00000000000..65f75260d6a --- /dev/null +++ b/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill @@ -0,0 +1,3 @@ +{ + "value" : "30000" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.width.max_per_query.sys.drill b/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.width.max_per_query.sys.drill new file mode 100644 index 00000000000..cd6c3856837 --- /dev/null +++ b/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.width.max_per_query.sys.drill @@ -0,0 +1,3 @@ +{ + "value" : "3333" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill b/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill new file mode 100644 index 00000000000..0b209765b49 --- /dev/null +++ b/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill @@ -0,0 +1,7 @@ +{ + "kind" : "LONG", + "type" : "SYSTEM", + "name" : "planner.store.parquet.rowgroup.filter.pushdown.threshold", + "num_val" : 30000, + "scope" : "SYSTEM" +} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.width.max_per_query.sys.drill b/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.width.max_per_query.sys.drill new file mode 100644 index 00000000000..e988f2d2b66 --- /dev/null +++ b/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.width.max_per_query.sys.drill @@ -0,0 +1,7 @@ +{ + "kind" : "LONG", + "type" : "SYSTEM", + "name" : "planner.width.max_per_query", + "num_val" : 3333, + "scope" : "SYSTEM" +} \ No newline at end of file diff --git a/pom.xml b/pom.xml index f22fafa3020..3583b98cbf1 100644 --- a/pom.xml +++ b/pom.xml @@ -232,6 +232,7 @@ **/*.httpd **/*.autotools **/*.cproject + **/*.drill dependency-reduced-pom.xml From 9c3a918a495abbba4612f76aa71e9fea9fa2c251 Mon Sep 17 00:00:00 2001 From: Timothy Farkas Date: Mon, 25 Sep 2017 12:40:21 -0700 Subject: [PATCH 2/7] - Fixed forward compatability issue --- .../drill/testutils/DirTestWatcher.java | 3 +- .../exec/server/options/OptionValue.java | 2 +- .../server/options/PersistedOptionValue.java | 205 +++++++++++++++++- .../options/PersistedOptionValueTest.java | 173 ++++++++++++++- .../exec/store/sys/TestPStoreProviders.java | 30 ++- .../resources/options/new_booleanopt.json | 3 - .../test/resources/options/new_doubleopt.json | 3 - .../test/resources/options/new_longopt.json | 3 - .../test/resources/options/new_stringopt.json | 3 - ...wgroup.filter.pushdown.threshold.sys.drill | 3 - .../planner.width.max_per_query.sys.drill | 3 - ...wgroup.filter.pushdown.threshold.sys.drill | 3 +- .../planner.width.max_per_query.sys.drill | 3 +- 13 files changed, 378 insertions(+), 59 deletions(-) delete mode 100644 exec/java-exec/src/test/resources/options/new_booleanopt.json delete mode 100644 exec/java-exec/src/test/resources/options/new_doubleopt.json delete mode 100644 exec/java-exec/src/test/resources/options/new_longopt.json delete mode 100644 exec/java-exec/src/test/resources/options/new_stringopt.json delete mode 100644 exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill delete mode 100644 exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.width.max_per_query.sys.drill diff --git a/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java b/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java index cad822d913c..93b952a2396 100644 --- a/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java +++ b/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java @@ -24,12 +24,11 @@ import java.io.File; -/** +/* * This JUnit {@link TestWatcher} creates a unique directory for each JUnit test in the project's * target folder at the start of each test. This directory can be used as a temporary directory to store * files for the test. The directory and its contents are deleted at the end of the test. */ - public class DirTestWatcher extends TestWatcher { private String dirPath; private File dir; diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java index 76d82c8c20c..e14a8460544 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java @@ -183,7 +183,7 @@ public Object getValue() { } public PersistedOptionValue toPersisted() { - return new PersistedOptionValue(getValue().toString()); + return new PersistedOptionValue(kind, name, num_val, string_val, bool_val, float_val); } @Override diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java index c65b681abee..0246cb2b651 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java @@ -18,6 +18,8 @@ package org.apache.drill.exec.server.options; +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonProcessingException; @@ -31,27 +33,176 @@ import java.io.IOException; +/** + *

+ * This represents a persisted {@link OptionValue}. Decoupling the {@link OptionValue} from what + * is persisted will prevent us from accidentally breaking backward compatibility in the future + * when the {@link OptionValue} changes. Additionally when we do change the format of stored options we + * will not have to change much code since this is already designed with backward compatibility in mind. + * This class is also forward compatible with the Drill Option storage format in Drill 1.11 and earlier. + *

+ * + *

+ * Contract: + * Only {@link PersistedOptionValue}s created from an {@link OptionValue} should be persisted. + * {@link PersistedOptionValue}s retrieved from a persisted store should never be re-persisted. + * And {@link OptionValue}s should only be created from {@link PersistedOptionValue}s that are + * retrieved from a store. + *

+ */ + // Custom Deserializer required for backward compatibility DRILL-5809 +@JsonInclude(JsonInclude.Include.NON_NULL) @JsonDeserialize(using = PersistedOptionValue.Deserializer.class) public class PersistedOptionValue { - public static final String JSON_VALUE = "value"; + /** + * This is present for forward compatability with Drill 1.11 and earlier + */ + public static final String SYSTEM_TYPE = "SYSTEM"; + /** + * This constant cannot be changed for backward and forward compatibility reasons. + */ + public static final String JSON_TYPE = "type"; + /** + * This constant cannot be changed for backward and forward compatibility reasons. + */ + public static final String JSON_KIND = "kind"; + /** + * This constant cannot be changed for backward and forward compatibility reasons. + */ + public static final String JSON_NAME = "name"; + /** + * This constant cannot be changed for backward and forward compatibility reasons. + */ + public static final String JSON_NUM_VAL = "num_val"; + /** + * This constant cannot be changed for backward and forward compatibility reasons. + */ + public static final String JSON_STRING_VAL = "string_val"; + /** + * This constant cannot be changed for backward and forward compatibility reasons. + */ + public static final String JSON_BOOL_VAL = "bool_val"; + /** + * This constant cannot be changed for backward and forward compatibility reasons. + */ + public static final String JSON_FLOAT_VAL = "float_val"; private String value; + private OptionValue.Kind kind; + private String name; + private Long num_val; + private String string_val; + private Boolean bool_val; + private Double float_val; - public PersistedOptionValue(@JsonProperty(JSON_VALUE) String value) { + public PersistedOptionValue(String value) { this.value = Preconditions.checkNotNull(value); } + public PersistedOptionValue(OptionValue.Kind kind, String name, + Long num_val, String string_val, + Boolean bool_val, Double float_val) { + this.kind = kind; + this.name = name; + this.num_val = num_val; + this.string_val = string_val; + this.bool_val = bool_val; + this.float_val = float_val; + + switch (kind) { + case BOOLEAN: + Preconditions.checkNotNull(bool_val); + value = bool_val.toString(); + break; + case STRING: + Preconditions.checkNotNull(string_val); + value = string_val; + break; + case DOUBLE: + Preconditions.checkNotNull(float_val); + value = float_val.toString(); + break; + case LONG: + Preconditions.checkNotNull(num_val); + value = num_val.toString(); + break; + default: + throw new UnsupportedOperationException(String.format("Unsupported type %s", kind)); + } + } + + /** + * This is ignored for forward compatibility. + */ + @JsonIgnore public String getValue() { return value; } + /** + * This is present for forward compatibility. + */ + @JsonProperty(JSON_TYPE) + public String getType() { + return SYSTEM_TYPE; + } + + /** + * This is present for forward compatibility. + */ + @JsonProperty(JSON_KIND) + public OptionValue.Kind getKind() { + return kind; + } + + /** + * This is present for forward compatibility. + */ + @JsonProperty(JSON_NAME) + public String getName() { + return name; + } + + /** + * This is present for forward compatibility. + */ + @JsonProperty(JSON_NUM_VAL) + public Long getNumVal() { + return num_val; + } + + /** + * This is present for forward compatibility. + */ + @JsonProperty(JSON_STRING_VAL) + public String getStringVal() { + return string_val; + } + + /** + * This is present for forward compatibility. + */ + @JsonProperty(JSON_BOOL_VAL) + public Boolean getBoolVal() { + return bool_val; + } + + /** + * This is present for forward compatibility. + */ + @JsonProperty(JSON_FLOAT_VAL) + public Double getFloatVal() { + return float_val; + } + public OptionValue toOptionValue(final OptionDefinition optionDefinition, final OptionValue.OptionScope optionScope) { + Preconditions.checkNotNull(value, "The value must be defined in order for this to be converted to an " + + "option value"); final OptionValidator validator = optionDefinition.getValidator(); final OptionValue.Kind kind = validator.getKind(); final String name = validator.getOptionName(); final OptionValue.AccessibleScopes accessibleScopes = optionDefinition.getMetaData().getAccessibleScopes(); - return OptionValue.create(kind, accessibleScopes, name, value, optionScope); } @@ -67,19 +218,57 @@ public boolean equals(Object o) { PersistedOptionValue that = (PersistedOptionValue) o; - return value.equals(that.value); + if (value != null ? !value.equals(that.value) : that.value != null) { + return false; + } + + if (kind != that.kind) { + return false; + } + + if (name != null ? !name.equals(that.name) : that.name != null) { + return false; + } + + if (num_val != null ? !num_val.equals(that.num_val) : that.num_val != null) { + return false; + } + + if (string_val != null ? !string_val.equals(that.string_val) : that.string_val != null) { + return false; + } + + if (bool_val != null ? !bool_val.equals(that.bool_val) : that.bool_val != null) { + return false; + } + + return float_val != null ? float_val.equals(that.float_val) : that.float_val == null; } @Override public int hashCode() { - return value.hashCode(); + int result = value != null ? value.hashCode() : 0; + result = 31 * result + (kind != null ? kind.hashCode() : 0); + result = 31 * result + (name != null ? name.hashCode() : 0); + result = 31 * result + (num_val != null ? num_val.hashCode() : 0); + result = 31 * result + (string_val != null ? string_val.hashCode() : 0); + result = 31 * result + (bool_val != null ? bool_val.hashCode() : 0); + result = 31 * result + (float_val != null ? float_val.hashCode() : 0); + return result; } @Override public String toString() { - return "PersistedOptionValue{" + "value='" + value + '\'' + '}'; + return "PersistedOptionValue{" + "value='" + value + '\'' + ", kind=" + kind + ", name='" + name + + '\'' + ", num_val=" + num_val + ", string_val='" + string_val + '\'' + ", bool_val=" + bool_val + + ", float_val=" + float_val + '}'; } + /** + * This deserializer only fetches the relevant information we care about from a store, which is the + * value of an option. This deserializer is essentially future proof since it only requires a value + * to be stored for an option. + */ public static class Deserializer extends StdDeserializer { private Deserializer() { super(PersistedOptionValue.class); @@ -115,10 +304,6 @@ public PersistedOptionValue deserialize(JsonParser p, DeserializationContext ctx value = node.get(OptionValue.JSON_FLOAT_VAL).asText(); } - if (node.has(JSON_VALUE)) { - value = node.get(JSON_VALUE).asText(); - } - return new PersistedOptionValue(value); } } diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java index bbfa8cd10d1..4c527ecffda 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java @@ -18,15 +18,24 @@ package org.apache.drill.exec.server.options; +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.drill.common.util.FileUtils; import org.apache.drill.exec.serialization.JacksonSerializer; +import org.apache.drill.exec.store.sys.PersistentStoreConfig; import org.junit.Assert; import org.junit.Test; import java.io.IOException; public class PersistedOptionValueTest { + /** + * DRILL-5809 + * Note: If this test breaks you are probably breaking backward and forward compatibility. Verify with the community + * that breaking compatibility is acceptable and planned for. + * @throws Exception + */ @Test public void oldDeserializeTest() throws IOException { testHelper("/options/old_booleanopt.json", @@ -35,14 +44,6 @@ public void oldDeserializeTest() throws IOException { "/options/old_stringopt.json"); } - @Test - public void newDeserializeTest() throws IOException { - testHelper("/options/new_booleanopt.json", - "/options/new_doubleopt.json", - "/options/new_longopt.json", - "/options/new_stringopt.json"); - } - private void testHelper(String booleanOptionFile, String doubleOptionFile, String longOptionFile, String stringOptionFile) throws IOException { JacksonSerializer serializer = new JacksonSerializer<>(new ObjectMapper(), PersistedOptionValue.class); @@ -66,4 +67,160 @@ private void testHelper(String booleanOptionFile, String doubleOptionFile, Assert.assertEquals(expectedLongValue, longValue); Assert.assertEquals(expectedStringValue, stringValue); } + + @Test + public void valueAssignment() { + final String name = "myOption"; + final String stringContent = "val1"; + PersistedOptionValue stringValue = + new PersistedOptionValue(OptionValue.Kind.STRING, name, null, stringContent, null, null); + PersistedOptionValue numValue = + new PersistedOptionValue(OptionValue.Kind.LONG, name, 100L, null, null, null); + PersistedOptionValue boolValue = + new PersistedOptionValue(OptionValue.Kind.BOOLEAN, name, null, null, true, null); + PersistedOptionValue floatValue = + new PersistedOptionValue(OptionValue.Kind.DOUBLE, name, null, null, null, 55.5); + + Assert.assertEquals(stringContent, stringValue.getValue()); + Assert.assertEquals("100", numValue.getValue()); + Assert.assertEquals("true", boolValue.getValue()); + Assert.assertEquals("55.5", floatValue.getValue()); + } + + /** + * DRILL-5809 Test forward compatibility with Drill 1.11 and earlier. + * Note: If this test breaks you are probably breaking forward compatibility. Verify with the community + * that breaking compatibility is acceptable and planned for. + * @throws Exception + */ + @Test + public void testForwardCompatibility() throws IOException { + final String name = "myOption"; + + JacksonSerializer realSerializer = new JacksonSerializer<>(new ObjectMapper(), PersistedOptionValue.class); + JacksonSerializer mockSerializer = new JacksonSerializer<>(new ObjectMapper(), MockPersistedOptionValue.class); + + final String stringContent = "val1"; + PersistedOptionValue stringValue = + new PersistedOptionValue(OptionValue.Kind.STRING, name, null, stringContent, null, null); + PersistedOptionValue numValue = + new PersistedOptionValue(OptionValue.Kind.LONG, name, 100L, null, null, null); + PersistedOptionValue boolValue = + new PersistedOptionValue(OptionValue.Kind.BOOLEAN, name, null, null, true, null); + PersistedOptionValue floatValue = + new PersistedOptionValue(OptionValue.Kind.DOUBLE, name, null, null, null, 55.5); + + byte[] stringValueBytes = realSerializer.serialize(stringValue); + byte[] numValueBytes = realSerializer.serialize(numValue); + byte[] boolValueBytes = realSerializer.serialize(boolValue); + byte[] floatValueBytes = realSerializer.serialize(floatValue); + + MockPersistedOptionValue mockStringValue = (MockPersistedOptionValue) mockSerializer.deserialize(stringValueBytes); + MockPersistedOptionValue mockNumValue = (MockPersistedOptionValue) mockSerializer.deserialize(numValueBytes); + MockPersistedOptionValue mockBoolValue = (MockPersistedOptionValue) mockSerializer.deserialize(boolValueBytes); + MockPersistedOptionValue mockFloatValue = (MockPersistedOptionValue) mockSerializer.deserialize(floatValueBytes); + + MockPersistedOptionValue expectedStringValue = + new MockPersistedOptionValue(PersistedOptionValue.SYSTEM_TYPE, OptionValue.Kind.STRING, name, + null, stringContent, null, null); + MockPersistedOptionValue expectedNumValue = + new MockPersistedOptionValue(PersistedOptionValue.SYSTEM_TYPE, OptionValue.Kind.LONG, name, + 100L, null, null, null); + MockPersistedOptionValue expectedBoolValue = + new MockPersistedOptionValue(PersistedOptionValue.SYSTEM_TYPE, OptionValue.Kind.BOOLEAN, name, + null, null, true, null); + MockPersistedOptionValue expectedFloatValue = + new MockPersistedOptionValue(PersistedOptionValue.SYSTEM_TYPE, OptionValue.Kind.DOUBLE, name, + null, null, null, 55.5); + + Assert.assertEquals(expectedStringValue, mockStringValue); + Assert.assertEquals(expectedNumValue, mockNumValue); + Assert.assertEquals(expectedBoolValue, mockBoolValue); + Assert.assertEquals(expectedFloatValue, mockFloatValue); + } + + @JsonInclude(JsonInclude.Include.NON_NULL) + public static class MockPersistedOptionValue { + public final String type; + public final OptionValue.Kind kind; + public final String name; + public final Long num_val; + public final String string_val; + public final Boolean bool_val; + public final Double float_val; + + public MockPersistedOptionValue(@JsonProperty(PersistedOptionValue.JSON_TYPE) String type, + @JsonProperty(PersistedOptionValue.JSON_KIND) OptionValue.Kind kind, + @JsonProperty(PersistedOptionValue.JSON_NAME) String name, + @JsonProperty(PersistedOptionValue.JSON_NUM_VAL) Long num_val, + @JsonProperty(PersistedOptionValue.JSON_STRING_VAL) String string_val, + @JsonProperty(PersistedOptionValue.JSON_BOOL_VAL) Boolean bool_val, + @JsonProperty(PersistedOptionValue.JSON_FLOAT_VAL) Double float_val) { + this.type = type; + this.kind = kind; + this.name = name; + this.num_val = num_val; + this.string_val = string_val; + this.bool_val = bool_val; + this.float_val = float_val; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + + if (o == null || getClass() != o.getClass()) { + return false; + } + + MockPersistedOptionValue that = (MockPersistedOptionValue) o; + + if (!type.equals(that.type)) { + return false; + } + + if (kind != that.kind) { + return false; + } + + if (!name.equals(that.name)) { + return false; + } + + if (num_val != null ? !num_val.equals(that.num_val) : that.num_val != null) { + return false; + } + + if (string_val != null ? !string_val.equals(that.string_val) : that.string_val != null) { + return false; + } + + if (bool_val != null ? !bool_val.equals(that.bool_val) : that.bool_val != null) { + return false; + } + + return float_val != null ? float_val.equals(that.float_val) : that.float_val == null; + } + + @Override + public int hashCode() { + int result = type.hashCode(); + result = 31 * result + kind.hashCode(); + result = 31 * result + name.hashCode(); + result = 31 * result + (num_val != null ? num_val.hashCode() : 0); + result = 31 * result + (string_val != null ? string_val.hashCode() : 0); + result = 31 * result + (bool_val != null ? bool_val.hashCode() : 0); + result = 31 * result + (float_val != null ? float_val.hashCode() : 0); + return result; + } + + @Override + public String toString() { + return "MockPersistedOptionValue{" + "type='" + type + '\'' + ", kind=" + kind + ", name='" + name + + '\'' + ", num_val=" + num_val + ", string_val='" + string_val + '\'' + ", bool_val=" + bool_val + + ", float_val=" + float_val + '}'; + } + } } diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/store/sys/TestPStoreProviders.java b/exec/java-exec/src/test/java/org/apache/drill/exec/store/sys/TestPStoreProviders.java index 91600d6752b..79a0f45ab4f 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/store/sys/TestPStoreProviders.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/store/sys/TestPStoreProviders.java @@ -65,12 +65,12 @@ public void verifyZkStore() throws Exception { } } - @Test - public void localLoadTest() throws Exception { - localLoadTestHelper("src/test/resources/options/store/local/new/sys.options"); - } - - // DRILL-5809 + /** + * DRILL-5809 + * Note: If this test breaks you are probably breaking backward and forward compatibility. Verify with the community + * that breaking compatibility is acceptable and planned for. + * @throws Exception + */ @Test public void localBackwardCompatabilityTest() throws Exception { localLoadTestHelper("src/test/resources/options/store/local/old/sys.options"); @@ -110,11 +110,15 @@ private void localLoadTestHelper(String propertiesDir) throws Exception { } } - // DRILL-5809 + /** + * DRILL-5809 + * Note: If this test breaks you are probably breaking backward and forward compatibility. Verify with the community + * that breaking compatibility is acceptable and planned for. + * @throws Exception + */ @Test public void zkBackwardCompatabilityTest() throws Exception { final String oldName = "myOldOption"; - final String newName = "myNewOption"; try (CuratorFramework curator = createCurator()) { curator.start(); @@ -126,9 +130,7 @@ public void zkBackwardCompatabilityTest() throws Exception { PathUtils.join("/", storeConfig.getName()), CreateMode.PERSISTENT)) { zkClient.start(); String oldOptionJson = FileUtils.getResourceAsString("/options/old_booleanopt.json"); - String newOptionJson = FileUtils.getResourceAsString("/options/new_booleanopt.json"); zkClient.put(oldName, oldOptionJson.getBytes(), null); - zkClient.put(newName, newOptionJson.getBytes(), null); } try (ZookeeperPersistentStoreProvider provider = @@ -136,13 +138,9 @@ public void zkBackwardCompatabilityTest() throws Exception { PersistentStore store = provider.getOrCreateStore(storeConfig); PersistedOptionValue oldOptionValue = store.get(oldName, null); - PersistedOptionValue newOptionValue = store.get(newName, null); - - PersistedOptionValue expectedOld = new PersistedOptionValue("true"); - PersistedOptionValue expectedNew = new PersistedOptionValue("true"); + PersistedOptionValue expectedValue = new PersistedOptionValue("true"); - Assert.assertEquals(expectedOld, oldOptionValue); - Assert.assertEquals(expectedNew, newOptionValue); + Assert.assertEquals(expectedValue, oldOptionValue); } } } diff --git a/exec/java-exec/src/test/resources/options/new_booleanopt.json b/exec/java-exec/src/test/resources/options/new_booleanopt.json deleted file mode 100644 index 302d1fd3b6f..00000000000 --- a/exec/java-exec/src/test/resources/options/new_booleanopt.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "value": "true" -} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/new_doubleopt.json b/exec/java-exec/src/test/resources/options/new_doubleopt.json deleted file mode 100644 index d6f89532804..00000000000 --- a/exec/java-exec/src/test/resources/options/new_doubleopt.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "value": "1.5" -} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/new_longopt.json b/exec/java-exec/src/test/resources/options/new_longopt.json deleted file mode 100644 index 186deac0f20..00000000000 --- a/exec/java-exec/src/test/resources/options/new_longopt.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "value": "5000" -} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/new_stringopt.json b/exec/java-exec/src/test/resources/options/new_stringopt.json deleted file mode 100644 index 64153096924..00000000000 --- a/exec/java-exec/src/test/resources/options/new_stringopt.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "value": "wabalubadubdub" -} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill b/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill deleted file mode 100644 index 65f75260d6a..00000000000 --- a/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill +++ /dev/null @@ -1,3 +0,0 @@ -{ - "value" : "30000" -} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.width.max_per_query.sys.drill b/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.width.max_per_query.sys.drill deleted file mode 100644 index cd6c3856837..00000000000 --- a/exec/java-exec/src/test/resources/options/store/local/new/sys.options/planner.width.max_per_query.sys.drill +++ /dev/null @@ -1,3 +0,0 @@ -{ - "value" : "3333" -} \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill b/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill index 0b209765b49..8d89a593b7a 100644 --- a/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill +++ b/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.store.parquet.rowgroup.filter.pushdown.threshold.sys.drill @@ -2,6 +2,5 @@ "kind" : "LONG", "type" : "SYSTEM", "name" : "planner.store.parquet.rowgroup.filter.pushdown.threshold", - "num_val" : 30000, - "scope" : "SYSTEM" + "num_val" : 30000 } \ No newline at end of file diff --git a/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.width.max_per_query.sys.drill b/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.width.max_per_query.sys.drill index e988f2d2b66..186d736aeb5 100644 --- a/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.width.max_per_query.sys.drill +++ b/exec/java-exec/src/test/resources/options/store/local/old/sys.options/planner.width.max_per_query.sys.drill @@ -2,6 +2,5 @@ "kind" : "LONG", "type" : "SYSTEM", "name" : "planner.width.max_per_query", - "num_val" : 3333, - "scope" : "SYSTEM" + "num_val" : 3333 } \ No newline at end of file From dd40d6c076fc0ebb4faa90f0a2c4e9881736d0f5 Mon Sep 17 00:00:00 2001 From: Timothy Farkas Date: Mon, 25 Sep 2017 18:10:36 -0700 Subject: [PATCH 3/7] - Added error message for debugging --- .../drill/exec/server/options/PersistedOptionValue.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java index 0246cb2b651..017b4aea0ae 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java @@ -27,6 +27,7 @@ import com.fasterxml.jackson.databind.DeserializationContext; import com.fasterxml.jackson.databind.JavaType; import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.deser.std.StdDeserializer; import com.google.common.base.Preconditions; @@ -270,6 +271,8 @@ public String toString() { * to be stored for an option. */ public static class Deserializer extends StdDeserializer { + private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(Deserializer.class); + private Deserializer() { super(PersistedOptionValue.class); } @@ -304,6 +307,10 @@ public PersistedOptionValue deserialize(JsonParser p, DeserializationContext ctx value = node.get(OptionValue.JSON_FLOAT_VAL).asText(); } + if (value == null) { + logger.error("Invalid json stored {}.", new ObjectMapper().writeValueAsString(node)); + } + return new PersistedOptionValue(value); } } From b693561c694ff5919ec6a0065e5803fe0904e0ee Mon Sep 17 00:00:00 2001 From: Timothy Farkas Date: Mon, 25 Sep 2017 19:04:50 -0700 Subject: [PATCH 4/7] - Fix flakey test --- .../drill/exec/testing/TestCountDownLatchInjection.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java b/exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java index 44cc3a710b8..407cb7c003f 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java @@ -122,7 +122,7 @@ public void run() { @Test // test would hang if the correct init, wait and countdowns did not happen, and the test timeout mechanism will // catch that case - public void latchInjected() { + public void latchInjected() throws InterruptedException { final int threads = 10; final ExtendedLatch trigger = new ExtendedLatch(1); final Pointer countingDownTime = new Pointer<>(); @@ -144,6 +144,11 @@ public void latchInjected() { fail("Thread should not be interrupted; there is no deliberate attempt."); return; } + + while (countingDownTime.value == null) { + Thread.sleep(100L); + } + assertTrue(timeSpentWaiting >= countingDownTime.value); try { queryContext.close(); From 11720e08b7d628ca2fe05eab5115618df57414a4 Mon Sep 17 00:00:00 2001 From: Timothy Farkas Date: Mon, 25 Sep 2017 19:13:47 -0700 Subject: [PATCH 5/7] - Cleaned up bad logging --- .../java/org/apache/drill/exec/util/MiniZooKeeperCluster.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/util/MiniZooKeeperCluster.java b/exec/java-exec/src/test/java/org/apache/drill/exec/util/MiniZooKeeperCluster.java index f9abe7f3b10..d0359e1e97c 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/util/MiniZooKeeperCluster.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/util/MiniZooKeeperCluster.java @@ -154,8 +154,6 @@ public int startup(File baseDir, int numZooKeeperServers) throws IOException, NIOServerCnxnFactory standaloneServerFactory; while (true) { - System.out.println("Starting zookeeper " + tentativePort); - try { standaloneServerFactory = new NIOServerCnxnFactory(); standaloneServerFactory.configure(new InetSocketAddress(tentativePort), 1000); @@ -171,7 +169,7 @@ public int startup(File baseDir, int numZooKeeperServers) throws IOException, try { standaloneServerFactory.startup(server); } catch (IOException e) { - LOG.error("Zookeeper startupt error", e); + LOG.error("Zookeeper startup error", e); tentativePort++; continue; } From 90442caf2a0e518e8daf3bc29dfcd6676b3329e5 Mon Sep 17 00:00:00 2001 From: Timothy Farkas Date: Mon, 25 Sep 2017 19:50:32 -0700 Subject: [PATCH 6/7] - Applied comments --- .../test/java/org/apache/drill/testutils/DirTestWatcher.java | 2 +- .../apache/drill/exec/server/options/PersistedOptionValue.java | 3 +-- .../org/apache/drill/exec/server/options/OptionValueTest.java | 2 +- .../drill/exec/server/options/PersistedOptionValueTest.java | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java b/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java index 93b952a2396..a1f974a0d90 100644 --- a/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java +++ b/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java @@ -24,7 +24,7 @@ import java.io.File; -/* +/** * This JUnit {@link TestWatcher} creates a unique directory for each JUnit test in the project's * target folder at the start of each test. This directory can be used as a temporary directory to store * files for the test. The directory and its contents are deleted at the end of the test. diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java index 017b4aea0ae..685799cb69d 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/PersistedOptionValue.java @@ -1,4 +1,4 @@ -/** +/* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information @@ -46,7 +46,6 @@ *

* Contract: * Only {@link PersistedOptionValue}s created from an {@link OptionValue} should be persisted. - * {@link PersistedOptionValue}s retrieved from a persisted store should never be re-persisted. * And {@link OptionValue}s should only be created from {@link PersistedOptionValue}s that are * retrieved from a store. *

diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/OptionValueTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/OptionValueTest.java index e590c506b4b..5f674e8b954 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/OptionValueTest.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/OptionValueTest.java @@ -1,4 +1,4 @@ -/** +/* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java index 4c527ecffda..182442bbfc0 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/server/options/PersistedOptionValueTest.java @@ -1,4 +1,4 @@ -/** +/* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information From 43468a61cd76289e2950f833da6bb9cf7b7085c7 Mon Sep 17 00:00:00 2001 From: Timothy Farkas Date: Mon, 25 Sep 2017 19:59:20 -0700 Subject: [PATCH 7/7] - Applied more comments --- .../test/java/org/apache/drill/testutils/DirTestWatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java b/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java index a1f974a0d90..a5a8806c5d8 100644 --- a/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java +++ b/common/src/test/java/org/apache/drill/testutils/DirTestWatcher.java @@ -1,4 +1,4 @@ -/** +/* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information