DRILL-6272: Refactor dynamic UDFs and function initializer tests to g… #1225

arina-ielchiieva · 2018-04-19T17:45:26Z

…enerate needed binary and source jars at runtime

Details in DRILL-6272.

arina-ielchiieva · 2018-04-19T17:45:37Z

@vrozov please review.

vrozov

Will it be easier to use (embedded) maven to create necessary jars?

vrozov · 2018-04-19T22:25:44Z

exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java

-    URL[] urls = {jars.resolve(binaryName).toUri().toURL(), jars.resolve(sourceName).toUri().toURL()};
+    String binaryName = "DrillUDF-1.0";
+    URL template = ClassLoader.getSystemClassLoader().getResource("udf/dynamic/CustomLowerFunctionTemplate");
+    assert template != null;


use junit assert in unit tests.

vrozov · 2018-04-19T22:28:12Z

exec/java-exec/src/test/resources/udf/dynamic/CustomAbsFunctionTemplate

@@ -0,0 +1,45 @@
+package org.apache.drill.udf.dynamic;


Apache license

vrozov · 2018-04-19T22:28:51Z

exec/java-exec/src/test/resources/udf/dynamic/CustomAbsFunctionTemplate

+    out.end = outputValue.getBytes().length;
+    buffer.setBytes(0, outputValue.getBytes());
+  }
+}


arina-ielchiieva · 2018-04-20T12:21:32Z

@vrozov addressed code review comments.
Regarding embedded maven, I have considered this option but using native Java to generate jars is more convenient. First of all, for the tests we need different jars, with and without resource file, classes. Doing it in Java allows to store less config and template files. Also we do not depend from any build tool. Plus it much easier to debug and run test using IDE or maven. I suggest we leave the current approach.

vrozov · 2018-04-20T13:49:12Z

@arina-ielchiieva I don't see why using maven embedder is a less preferable option. Using maven it is possible to create source jars using standard maven plugin. IMO, it will be easier to modify test UDF project if necessary and it is still possible to debug and run tests using IDE or maven. There will be no dependency on a build tool, it is a dependency on an external jar similar to any other external jar dependency.

arina-ielchiieva · 2018-04-23T09:30:30Z

@vrozov I'll re-work PR with maven embedder, thanks for the idea. I'll ping you when changes are done.

arina-ielchiieva · 2018-04-25T19:48:12Z

@vrozov re-implemented using maven embedder. Had to upgrade jmokcit lib to the latest version since it caused NPE with maven embedder (NPE from jmockit even if no mocks are used, fixed in newer version). Please review.

vrozov · 2018-04-26T00:51:04Z

pom.xml

@@ -798,7 +798,7 @@
      <!-- JMockit needs to be on class path before JUnit. -->
      <groupId>com.googlecode.jmockit</groupId>
      <artifactId>jmockit</artifactId>
-      <version>1.3</version>
+      <version>1.7</version>


Can it be done as a precursor PR? 1.7 version is quite old too. Can it be upgraded to the latest (org.jmockit:jmockit:1.39)?

vrozov · 2018-04-26T14:33:12Z

exec/java-exec/pom.xml

+    <dependency>
+      <groupId>org.apache.maven</groupId>
+      <artifactId>maven-embedder</artifactId>
+      <version>3.3.9</version>


Consider using the latest release available.

arina-ielchiieva · 2018-04-27T15:58:29Z

@vrozov now PR contains two commits:

jmockit and mockito upgrade (DRILL-6363);
maven-embedder usage for unit tests (used latest version as you suggested) (DRILL-6272).
Please review.

vrozov · 2018-05-01T22:41:30Z

exec/java-exec/src/test/java/org/apache/drill/TestTpchDistributedConcurrent.java

@@ -177,7 +177,7 @@ public void run() {
    }
  }

-  //@Test


What is the reason the test was disabled before?

I have accidentally disabled in my previous PR. Did not want to create separate Jira for one line fix so included it here.

vrozov · 2018-05-02T00:36:11Z

exec/java-exec/src/test/java/org/apache/drill/exec/coord/zk/TestZookeeperClient.java

@@ -125,7 +125,7 @@ public void testHasPathThrowsDrillRuntimeException() {

    Mockito
        .when(client.getCache().getCurrentData(absPath))
-        .thenThrow(Exception.class);
+        .thenThrow(RuntimeException.class);


OK, but I am not sure what does this method test. ZookeeperClient.hasPath(String path) is not used in production.

I just fixed this test to work with new mockito lib version. I suspect this test checks if exception thrown from client.getCache().getCurrentData(absPath) method will be wrapped in DrillRuntimeException.

IMO, this method needs to be changed to test ZookeeperClient.hasPath(String path, boolean consistent). It is OK to do it in this PR or in a separate commit/JIRA/PR. If you decide to do it in a separate commit, please file JIRA.

Let's do it in different PR.

vrozov · 2018-05-02T01:08:44Z

exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java

  @BeforeClass
  public static void init() throws Exception {
-    MockUp<UUID> uuidMockUp = mockRandomUUID(session_id);


It is hard to say whether you want to change test logic or need a workaround for the removed MockUp.tearDown() method. In the latter case consider adding getTemporaryName() method to the UserSession class. The method can delegate to UUID.randomUUID() in production and can be mocked to return a predefined value in tests like this one. It will allow not to mock UUID.randomUUID() at all.

I had to re-write this part since MockUp.tearDown() is not present in newer jmockit version. Now we don't mock session id at all.

UUID.randomUUID() was mocked both for session id and temporary table name. Can you get temporary table name the same way you do it for session id?

Yes, from user session: userSession.resolveTemporaryTableName(tableName).

vrozov · 2018-05-02T01:16:26Z

exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java

-    }
+    String temporaryTableName = "temporary_table_outside_of_default_workspace";
+
+    thrown.expect(UserRemoteException.class);


Consider introducing a new method to set thrown and message, something like void expectUserRemoteExceptionWithMessage(String message).

vrozov · 2018-05-02T01:22:31Z

exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java

+
+    thrown.expect(UserRemoteException.class);
+    thrown.expectMessage(containsString(String.format(
+        "VALIDATION ERROR: A table or view with given name [%s]" +


and possibly expectUserRemoteExceptionWithTableExistsMessage(String tableName, String schemaName).

vrozov · 2018-05-02T01:29:38Z

exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java

+    test("drop view %s.%s", DFS_TMP_SCHEMA, temporaryTableName);
+  }
+
+  private static String getSessionId() throws Exception {


Consider mocking getSessionId() in the UserSession. This method needs to be tested by itself.

Re-written, it turned out I have access to UserSession through DrillbitContext, so no mock is required at all.

vrozov · 2018-05-02T02:53:40Z

exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/JarBuilder.java

+    return cli.doMain(params.toArray(new String[params.size()]), projectDir, System.out, System.err);
+  }
+
+  private static Logger setupLogger(String string, Level logLevel) {


Is this necessary?

Yes, since I want to output info level logging when creating jars. Otherwise, no logs on how jars were created will appear.

It should depend on log level and not be forcefully overridden. To debug, you have two options, either add "-X" to maven arguments or change log level for that logger in logback-test.xml.

Agree, removed.

vrozov · 2018-05-02T03:08:51Z

exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java

-    String sourceName = JarUtil.getSourceName(binaryName);
-    URL[] urls = {jars.resolve(binaryName).toUri().toURL(), jars.resolve(sourceName).toUri().toURL()};
+    File projectDir = dirTestWatcher.makeSubDir(Paths.get("drill-udf"));
+    FileUtils.copyDirectory(getResourceFile(Paths.get(projectDir.getName())), projectDir);


Consider using <directory> in pom.xml if the goal here is to avoid creating files in the src path.

Done, thanks.

vrozov · 2018-05-02T03:14:34Z

exec/java-exec/src/test/resources/drill-udf/pom.xml

+
+  <properties>
+    <jar.finalName>${project.name}</jar.finalName>
+    <drill.version>1.13.0</drill.version>


Is it OK to use old version? Does Drill support semver API compatibility for UDFs? If yes, how is it enforced? If no, compilation may fail.

My previous jars used 1.8.0. for example, since I created them when that only version was available. API for Drill UDFs is public so we should not change it. There is no enforcement though.

What is a reason not to use the SNAPSHOT version? Let's say somebody wants to introduce a new API for UDF and test it.

Used DrillVersionInfo.getVersion() to pas current Drill project version to the build.

arina-ielchiieva · 2018-05-02T12:21:55Z

@vrozov addressed code review comments in the new commit. Please review.

vrozov · 2018-05-02T16:15:02Z

exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java

 @Category(SqlFunctionTest.class)
 public class FunctionInitializerTest {

-  private static final String CLASS_NAME = "com.drill.udf.CustomLowerFunction";
+  @ClassRule
+  public static final BaseDirTestWatcher dirTestWatcher = new BaseDirTestWatcher();


Sorry, missed that in my previous review. Consider using TemporaryFolder rule instead of BaseDirTestWatcher.

Makes sense, fixed.

vrozov · 2018-05-02T16:42:43Z

exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/FunctionInitializerTest.java

+
+    JarBuilder jarBuilder = new JarBuilder("src/test/resources/drill-udf");
+    int result = jarBuilder.build(binaryName, buildDirectory.getAbsolutePath(), "**/CustomLowerFunction.java", null);
+    assertEquals("Build should be successful.", 0, result);


Consider moving assertEquals inside JarBuilder.build() and returning name of the binary jar instead.

vrozov · 2018-05-02T16:59:53Z

exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java

-      throw e;
-    }
+
+    expectUserRemoteExceptionWithTableExistsMessage(temporaryTableName, DFS_TMP_SCHEMA);


Move it between test calls. My understanding the first one should succeed without exception.

Likely the same pattern applies to other unit tests with mulitple calls to test.

Agree, fixed.

vrozov · 2018-05-02T17:29:35Z

exec/java-exec/src/test/resources/drill-udf/pom.xml

+      <groupId>org.apache.drill.exec</groupId>
+      <artifactId>drill-java-exec</artifactId>
+      <version>${drill.version}</version>
+      <scope>compile</scope>


Should it be provided?

vrozov · 2018-05-02T17:38:34Z

exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/JarBuilder.java

+   * @return build exit code, 0 if build was successful
+   */
+  public int build(String jarName, String buildDirectory, String includeFiles, String includeResources) {
+    System.setProperty("maven.multiModuleProjectDirectory", projectDirectory);


Check if the property already set. It probably will be good to restore the property to it's original value.

arina-ielchiieva · 2018-05-03T16:35:07Z

@vrozov addressed code review comments in two new commits. Please review.

vrozov

Mostly LGTM (see few minor comments). Rebase and squash to two commits.

vrozov · 2018-05-03T21:18:04Z

exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTTAS.java

-      throw e;
-    }
+
+    expectUserRemoteExceptionWithTableExistsMessage(temporaryTableName, DFS_TMP_SCHEMA);


Please move this one too.

vrozov · 2018-05-03T21:24:55Z

exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/JarBuilder.java

+      assertEquals("Build should be successful.", 0, result);
+      return jarName + ".jar";
+    } finally {
+      if (originalPropertyValue != null) {


Consider originalPropertyValue != null ? System.setProperty(MAVEN_MULTI_MODULE_PROJECT_DIRECTORY, originalPropertyValue) : System.clearProperty(MAVEN_MULTI_MODULE_PROJECT_DIRECTORY);.

vrozov · 2018-05-03T22:07:55Z

exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/TestDynamicUDFSupport.java

@@ -101,6 +110,9 @@ public static void setup() throws IOException {
    fsUri = getLocalFileSystem().getUri();
  }

+  @Rule
+  public ExpectedException thrown = ExpectedException.none();
+
  @Rule


Is the rule necessary? Can @After provide the same functionality? If yes, please file JIRA.

vrozov · 2018-05-03T23:14:22Z

exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/TestDynamicUDFSupport.java


+  @BeforeClass


Consider @Before instead of calling it from reset().

arina-ielchiieva · 2018-05-05T16:29:46Z

@vrozov done all the changes. One point regarding using current drill snapshot version as dependency when building jars. I decided to leave it hard-coded to 1.13.0 because mvn clean install with clean local maven repo succeeds since 1.13.0 is downloaded from central mvn repo. If we use 1.14.0-SNAPSHOT, build fails because this version is absent in central maven repo and in local, in other words to run unit tests on new snapshot version, we would have to build the project first which is not acceptable especially since currently we are not obliged to it.
Left the following comment in code, in case somebody what to build jars with current version:

      // uncomment to build with current Drill version
      // params.add("-Ddrill.version=" + DrillVersionInfo.getVersion());

vrozov · 2018-05-07T01:27:46Z

...java-exec/src/test/java/org/apache/drill/exec/rpc/user/TemporaryTablesAutomaticDropTest.java

 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;

 public class TemporaryTablesAutomaticDropTest extends BaseTestQuery {

  private static final String session_id = "sessionId";

+  private static FileSystem fs;


Can fs and permissions be declared as local variables?

Nope, they are defined in @BeforeClass and the same for all tests.

vrozov · 2018-05-07T01:34:27Z

...java-exec/src/test/java/org/apache/drill/exec/rpc/user/TemporaryTablesAutomaticDropTest.java

  @Before
-  public void setup() throws Exception {
+  public void setup() {


Is it necessary to mock UUID?. You mentioned that there is a way to get sessionId from UserSession.

Unfortunately, yes. It turned out that there is no good way to retrieve session information in tests. Sorry for confusion.

vrozov · 2018-05-07T01:35:48Z

...java-exec/src/test/java/org/apache/drill/exec/rpc/user/TemporaryTablesAutomaticDropTest.java

 import java.util.UUID;

 import static org.apache.drill.exec.util.StoragePluginTestUtils.DFS_TMP_SCHEMA;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;

 public class TemporaryTablesAutomaticDropTest extends BaseTestQuery {

  private static final String session_id = "sessionId";


In case mocking is required, consider replacing String with UUID: uuid = UUID.nameUUIDFromBytes(session_id.getBytes())

Replaced it to UUID.randomUUID().

vrozov · 2018-05-07T01:54:09Z

exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/JarBuilder.java

+      assertEquals("Build should be successful.", 0, result);
+      return jarName + ".jar";
+    } finally {
+       if (originalPropertyValue != null) {


Please fix indentation.

vrozov · 2018-05-07T01:59:40Z

exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/JarBuilder.java

+  public String build(String jarName, String buildDirectory, String includeFiles, String includeResources) {
+    String originalPropertyValue = null;
+    try {
+      originalPropertyValue = System.setProperty(MAVEN_MULTI_MODULE_PROJECT_DIRECTORY, projectDirectory);


Do not split into two assignments. Move System.setProperty() call outside of try/finally block.

arina-ielchiieva

@vrozov done the changes.

arina-ielchiieva · 2018-05-07T16:13:26Z

exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/JarBuilder.java

+  public String build(String jarName, String buildDirectory, String includeFiles, String includeResources) {
+    String originalPropertyValue = null;
+    try {
+      originalPropertyValue = System.setProperty(MAVEN_MULTI_MODULE_PROJECT_DIRECTORY, projectDirectory);


arina-ielchiieva · 2018-05-07T16:16:19Z

...java-exec/src/test/java/org/apache/drill/exec/rpc/user/TemporaryTablesAutomaticDropTest.java

 import java.util.UUID;

 import static org.apache.drill.exec.util.StoragePluginTestUtils.DFS_TMP_SCHEMA;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;

 public class TemporaryTablesAutomaticDropTest extends BaseTestQuery {

  private static final String session_id = "sessionId";


Replaced it to UUID.randomUUID().

arina-ielchiieva · 2018-05-07T16:17:49Z

...java-exec/src/test/java/org/apache/drill/exec/rpc/user/TemporaryTablesAutomaticDropTest.java

 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;

 public class TemporaryTablesAutomaticDropTest extends BaseTestQuery {

  private static final String session_id = "sessionId";

+  private static FileSystem fs;


Nope, they are defined in @BeforeClass and the same for all tests.

arina-ielchiieva · 2018-05-07T16:18:40Z

...java-exec/src/test/java/org/apache/drill/exec/rpc/user/TemporaryTablesAutomaticDropTest.java

  @Before
-  public void setup() throws Exception {
+  public void setup() {


Unfortunately, yes. It turned out that there is no good way to retrieve session information in tests. Sorry for confusion.

arina-ielchiieva · 2018-05-07T16:19:34Z

exec/java-exec/src/test/java/org/apache/drill/exec/udf/dynamic/JarBuilder.java

+      assertEquals("Build should be successful.", 0, result);
+      return jarName + ".jar";
+    } finally {
+       if (originalPropertyValue != null) {


vrozov

LGTM.

amansinha100 · 2018-05-11T22:26:24Z

@arina-ielchiieva I encountered some merge conflicts. Can you pls rebase on master ?

arina-ielchiieva · 2018-05-12T14:55:39Z

@amansinha100 done.

vrozov · 2018-05-12T15:31:24Z

common/src/test/java/org/apache/drill/test/TestTools.java

@@ -47,7 +48,7 @@
    .indexOf("-agentlib:jdwp") > 0;

  public static TestRule getTimeoutRule(int timeout) {
-    return IS_DEBUG ? new TestName() : new Timeout(timeout);
+    return IS_DEBUG ? new TestName() : new Timeout(timeout, TimeUnit.MILLISECONDS);


Timeout.millis(timeout)?

Sure, fixed.

…enerate needed binary and source jars at runtime

vrozov · 2018-05-12T16:22:04Z

LGTM

vrozov reviewed Apr 19, 2018

View reviewed changes

arina-ielchiieva force-pushed the DRILL-6272 branch 2 times, most recently from d17439f to ac2c97d Compare April 20, 2018 12:05

arina-ielchiieva force-pushed the DRILL-6272 branch from ac2c97d to 7ac9f12 Compare April 25, 2018 19:45

vrozov reviewed Apr 26, 2018

View reviewed changes

arina-ielchiieva force-pushed the DRILL-6272 branch from 7ac9f12 to 2169aa2 Compare April 27, 2018 15:56

vrozov reviewed May 2, 2018

View reviewed changes

arina-ielchiieva force-pushed the DRILL-6272 branch from f96c741 to fc7aac5 Compare May 3, 2018 16:30

vrozov approved these changes May 3, 2018

View reviewed changes

arina-ielchiieva force-pushed the DRILL-6272 branch from fc7aac5 to 9834869 Compare May 5, 2018 15:29

vrozov reviewed May 7, 2018

View reviewed changes

arina-ielchiieva force-pushed the DRILL-6272 branch from 9834869 to 99e2cf3 Compare May 7, 2018 16:33

arina-ielchiieva commented May 7, 2018

View reviewed changes

vrozov approved these changes May 7, 2018

View reviewed changes

arina-ielchiieva force-pushed the DRILL-6272 branch 2 times, most recently from b5fc082 to 166a7b1 Compare May 11, 2018 15:36

arina-ielchiieva force-pushed the DRILL-6272 branch from 166a7b1 to 4b65235 Compare May 12, 2018 14:05

vrozov reviewed May 12, 2018

View reviewed changes

arina-ielchiieva added 2 commits May 12, 2018 19:12

DRILL-6363: Upgrade jmockit and mockito libs

8fe8f2c

DRILL-6272: Refactor dynamic UDFs and function initializer tests to g…

670f252

…enerate needed binary and source jars at runtime

arina-ielchiieva force-pushed the DRILL-6272 branch from 4b65235 to 670f252 Compare May 12, 2018 16:15

amansinha100 closed this in ca90229 May 12, 2018

@@ @@ -177,7 +177,7 @@ public void run() { @@
                   }
                 }
-                //@Test

DRILL-6272: Refactor dynamic UDFs and function initializer tests to g… #1225

DRILL-6272: Refactor dynamic UDFs and function initializer tests to g… #1225

Conversation

arina-ielchiieva commented Apr 19, 2018

arina-ielchiieva commented Apr 19, 2018

vrozov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arina-ielchiieva commented Apr 20, 2018

vrozov commented Apr 20, 2018

arina-ielchiieva commented Apr 23, 2018

arina-ielchiieva commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arina-ielchiieva commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arina-ielchiieva commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arina-ielchiieva commented May 3, 2018

vrozov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arina-ielchiieva commented May 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arina-ielchiieva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment