You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@drill.apache.org by jn...@apache.org on 2017/06/03 04:46:00 UTC
[05/12] drill git commit: DRILL-5533: Fix flag assignment in
FunctionInitializer.checkInit() method
DRILL-5533: Fix flag assignment in FunctionInitializer.checkInit() method
Changes:
1. Fixed DCL in FunctionInitializer.checkInit() method (update flag parameter when function body is loaded).
2. Fixed ImportGrabber.getImports() method to return the list with imports.
3. Added unit tests for FunctionInitializer.
4. Minor refactoring (renamed methods, added javadoc).
closes #843
Project: http://git-wip-us.apache.org/repos/asf/drill/repo
Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/155820a4
Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/155820a4
Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/155820a4
Branch: refs/heads/master
Commit: 155820a49563d631cbafd61a8538619ced21bd95
Parents: 62326be
Author: Arina Ielchiieva <ar...@gmail.com>
Authored: Mon May 22 17:49:31 2017 +0300
Committer: Jinfeng Ni <jn...@apache.org>
Committed: Fri Jun 2 21:43:14 2017 -0700
----------------------------------------------------------------------
.../drill/exec/expr/fn/FunctionInitializer.java | 70 +++++------
.../drill/exec/expr/fn/ImportGrabber.java | 27 ++--
.../exec/expr/fn/FunctionInitializerTest.java | 124 +++++++++++++++++++
3 files changed, 178 insertions(+), 43 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/drill/blob/155820a4/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java
index 4e5ee4f..9ca6dbd 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionInitializer.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
@@ -31,21 +31,18 @@ import org.codehaus.janino.Parser;
import org.codehaus.janino.Scanner;
import org.mortbay.util.IO;
-import com.google.common.collect.Maps;
-
/**
* To avoid the cost of initializing all functions up front,
- * this class contains all informations required to initializing a function when it is used.
+ * this class contains all information required to initializing a function when it is used.
*/
public class FunctionInitializer {
- static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FunctionInitializer.class);
+ private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FunctionInitializer.class);
private final String className;
private final ClassLoader classLoader;
- private Map<String, CompilationUnit> functionUnits = Maps.newHashMap();
private Map<String, String> methods;
private List<String> imports;
- private volatile boolean ready;
+ private volatile boolean isLoaded;
/**
* @param className the fully qualified name of the class implementing the function
@@ -53,7 +50,6 @@ public class FunctionInitializer {
* to prevent classpath collisions during loading an unloading jars
*/
public FunctionInitializer(String className, ClassLoader classLoader) {
- super();
this.className = className;
this.classLoader = classLoader;
}
@@ -74,41 +70,43 @@ public class FunctionInitializer {
* @return the imports of this class (for java code gen)
*/
public List<String> getImports() {
- checkInit();
+ loadFunctionBody();
return imports;
}
/**
- * @param methodName
+ * @param methodName method name
* @return the content of the method (for java code gen inlining)
*/
public String getMethod(String methodName) {
- checkInit();
+ loadFunctionBody();
return methods.get(methodName);
}
- private void checkInit() {
- if (ready) {
+ /**
+ * Loads function body: methods (for instance, eval, setup, reset) and imports.
+ * Loading is done once per class instance upon first function invocation.
+ * Double-checked locking is used to avoid concurrency issues
+ * when two threads are trying to load the function body at the same time.
+ */
+ private void loadFunctionBody() {
+ if (isLoaded) {
return;
}
synchronized (this) {
- if (ready) {
+ if (isLoaded) {
return;
}
- // get function body.
-
+ logger.trace("Getting function body for the {}", className);
try {
final Class<?> clazz = Class.forName(className, true, classLoader);
- final CompilationUnit cu = get(clazz);
-
- if (cu == null) {
- throw new IOException(String.format("Failure while loading class %s.", clazz.getName()));
- }
+ final CompilationUnit cu = convertToCompilationUnit(clazz);
methods = MethodGrabbingVisitor.getMethods(cu, clazz);
- imports = ImportGrabber.getMethods(cu);
+ imports = ImportGrabber.getImports(cu);
+ isLoaded = true;
} catch (IOException | ClassNotFoundException e) {
throw UserException.functionError(e)
@@ -119,20 +117,25 @@ public class FunctionInitializer {
}
}
- private CompilationUnit get(Class<?> c) throws IOException {
- String path = c.getName();
+ /**
+ * Using class name generates path to class source code (*.java),
+ * reads its content as string and parses it into {@link org.codehaus.janino.Java.CompilationUnit}.
+ *
+ * @param clazz function class
+ * @return compilation unit
+ * @throws IOException if did not find class or could not load it
+ */
+ private CompilationUnit convertToCompilationUnit(Class<?> clazz) throws IOException {
+ String path = clazz.getName();
path = path.replaceFirst("\\$.*", "");
path = path.replace(".", FileUtils.separator);
path = "/" + path + ".java";
- CompilationUnit cu = functionUnits.get(path);
- if (cu != null) {
- return cu;
- }
- try (InputStream is = c.getResourceAsStream(path)) {
+ logger.trace("Loading function code from the {}", path);
+ try (InputStream is = clazz.getResourceAsStream(path)) {
if (is == null) {
throw new IOException(String.format(
- "Failure trying to located source code for Class %s, tried to read on classpath location %s", c.getName(),
+ "Failure trying to locate source code for class %s, tried to read on classpath location %s", clazz.getName(),
path));
}
String body = IO.toString(is);
@@ -140,12 +143,9 @@ public class FunctionInitializer {
// TODO: Hack to remove annotations so Janino doesn't choke. Need to reconsider this problem...
body = body.replaceAll("@\\w+(?:\\([^\\\\]*?\\))?", "");
try {
- cu = new Parser(new Scanner(null, new StringReader(body))).parseCompilationUnit();
- functionUnits.put(path, cu);
- return cu;
+ return new Parser(new Scanner(null, new StringReader(body))).parseCompilationUnit();
} catch (CompileException e) {
- logger.warn("Failure while parsing function class:\n{}", body, e);
- return null;
+ throw new IOException(String.format("Failure while loading class %s.", clazz.getName()), e);
}
}
http://git-wip-us.apache.org/repos/asf/drill/blob/155820a4/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ImportGrabber.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ImportGrabber.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ImportGrabber.java
index d87e6fa..1437818 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ImportGrabber.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ImportGrabber.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
@@ -29,16 +29,15 @@ import org.codehaus.janino.util.Traverser;
import com.google.common.collect.Lists;
-public class ImportGrabber{
- static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(ImportGrabber.class);
+public class ImportGrabber {
- private List<String> imports = Lists.newArrayList();
+ private final List<String> imports = Lists.newArrayList();
private final ImportFinder finder = new ImportFinder();
private ImportGrabber() {
}
- public class ImportFinder extends Traverser{
+ public class ImportFinder extends Traverser {
@Override
public void traverseSingleTypeImportDeclaration(SingleTypeImportDeclaration stid) {
@@ -63,9 +62,21 @@ public class ImportGrabber{
}
- public static List<String> getMethods(Java.CompilationUnit cu){
- ImportGrabber visitor = new ImportGrabber();
- cu.getPackageMemberTypeDeclarations()[0].accept(visitor.finder.comprehensiveVisitor());
+ /**
+ * Creates list of imports that are present in compilation unit.
+ * For example:
+ * [import io.netty.buffer.DrillBuf;, import org.apache.drill.exec.expr.DrillSimpleFunc;]
+ *
+ * @param compilationUnit compilation unit
+ * @return list of imports
+ */
+ public static List<String> getImports(Java.CompilationUnit compilationUnit){
+ final ImportGrabber visitor = new ImportGrabber();
+
+ for (Java.CompilationUnit.ImportDeclaration importDeclaration : compilationUnit.importDeclarations) {
+ importDeclaration.accept(visitor.finder.comprehensiveVisitor());
+ }
+
return visitor.imports;
}
http://git-wip-us.apache.org/repos/asf/drill/blob/155820a4/exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java
new file mode 100644
index 0000000..2151095
--- /dev/null
+++ b/exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java
@@ -0,0 +1,124 @@
+/*
+ * 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.expr.fn;
+
+import com.google.common.collect.Lists;
+import mockit.Invocation;
+import mockit.Mock;
+import mockit.MockUp;
+import mockit.integration.junit4.JMockit;
+import org.apache.drill.common.util.TestTools;
+import org.apache.drill.exec.util.JarUtil;
+import org.codehaus.janino.Java;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import java.io.File;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+
+@RunWith(JMockit.class)
+public class FunctionInitializerTest {
+
+ private static final String CLASS_NAME = "com.drill.udf.CustomLowerFunction";
+ private static URLClassLoader classLoader;
+
+ @BeforeClass
+ public static void init() throws Exception {
+ File jars = new File(TestTools.getWorkingPath(), "/src/test/resources/jars");
+ String binaryName = "DrillUDF-1.0.jar";
+ String sourceName = JarUtil.getSourceName(binaryName);
+ URL[] urls = {new File(jars, binaryName).toURI().toURL(), new File(jars, sourceName).toURI().toURL()};
+ classLoader = new URLClassLoader(urls);
+ }
+
+ @Test
+ public void testGetImports() {
+ FunctionInitializer functionInitializer = new FunctionInitializer(CLASS_NAME, classLoader);
+ List<String> actualImports = functionInitializer.getImports();
+
+ List<String> expectedImports = Lists.newArrayList(
+ "import io.netty.buffer.DrillBuf;",
+ "import org.apache.drill.exec.expr.DrillSimpleFunc;",
+ "import org.apache.drill.exec.expr.annotations.FunctionTemplate;",
+ "import org.apache.drill.exec.expr.annotations.Output;",
+ "import org.apache.drill.exec.expr.annotations.Param;",
+ "import org.apache.drill.exec.expr.holders.VarCharHolder;",
+ "import javax.inject.Inject;"
+ );
+
+ assertEquals("List of imports should match", expectedImports, actualImports);
+ }
+
+ @Test
+ public void testGetMethod() {
+ FunctionInitializer functionInitializer = new FunctionInitializer(CLASS_NAME, classLoader);
+ String actualMethod = functionInitializer.getMethod("eval");
+ assertTrue("Method body should match", actualMethod.contains("CustomLowerFunction_eval:"));
+ }
+
+ @Test
+ public void testConcurrentFunctionBodyLoad() throws Exception {
+ final FunctionInitializer functionInitializer = new FunctionInitializer(CLASS_NAME, classLoader);
+
+ final AtomicInteger counter = new AtomicInteger();
+ new MockUp<FunctionInitializer>() {
+ @Mock
+ Java.CompilationUnit convertToCompilationUnit(Invocation inv, Class<?> clazz) {
+ counter.incrementAndGet();
+ return inv.proceed();
+ }
+ };
+
+ int threadsNumber = 5;
+ ExecutorService executor = Executors.newFixedThreadPool(threadsNumber);
+
+ try {
+ List<Future<String>> results = executor.invokeAll(Collections.nCopies(threadsNumber, new Callable<String>() {
+ @Override
+ public String call() throws Exception {
+ return functionInitializer.getMethod("eval");
+ }
+ }));
+
+ final Set<String> uniqueResults = new HashSet<>();
+ for (Future<String> result : results) {
+ uniqueResults.add(result.get());
+ }
+
+ assertEquals("All threads should have received the same result", 1, uniqueResults.size());
+ assertEquals("Number of function body loads should match", 1, counter.intValue());
+
+ } finally {
+ executor.shutdownNow();
+ }
+ }
+}