Mailing-List: contact commits-help@drill.apache.org; run by ezmlm
Precedence: bulk
Reply-To: commits@drill.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: sudheesh@apache.org
To: commits@drill.apache.org
Date: Tue, 13 Dec 2016 00:40:02 -0000
Message-Id: <503bd1d939e2490dba4c4abdf5842d20@git.apache.org>
In-Reply-To: <2579c1c039c64509be427ffbd9cc850b@git.apache.org>
References: <2579c1c039c64509be427ffbd9cc850b@git.apache.org>
Subject: [05/14] drill git commit: DRILL-5044: Fix retry logic to handle
 VersionMismatchException by not deleting jars in remote UDFs area
archived-at: Tue, 13 Dec 2016 00:40:01 -0000

DRILL-5044: Fix retry logic to handle VersionMismatchException by not deleting jars in remote UDFs area

closes #669


Project: http://git-wip-us.apache.org/repos/asf/drill/repo
Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/fb32bfe2
Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/fb32bfe2
Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/fb32bfe2

Branch: refs/heads/master
Commit: fb32bfe209b03ff269313aed70bf066b29314327
Parents: 85a9651
Author: Arina Ielchiieva <arina.yelchiyeva@gmail.com>
Authored: Fri Nov 25 16:44:08 2016 +0000
Committer: Sudheesh Katkam <sudheesh@apache.org>
Committed: Mon Dec 12 15:40:02 2016 -0800

----------------------------------------------------------------------
 .../sql/handlers/CreateFunctionHandler.java     | 54 ++++++-----
 .../sql/handlers/DropFunctionHandler.java       | 47 +++++-----
 .../org/apache/drill/TestDynamicUDFSupport.java | 96 +++++++++++++++++++-
 3 files changed, 146 insertions(+), 51 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/drill/blob/fb32bfe2/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateFunctionHandler.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateFunctionHandler.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateFunctionHandler.java
index 8515c8a..48bfd8b 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateFunctionHandler.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateFunctionHandler.java
@@ -90,7 +90,7 @@ public class CreateFunctionHandler extends DefaultSqlHandler {
 
       jarManager.initRemoteBackup();
       List<String> functions = validateAgainstLocalRegistry(jarManager, context.getFunctionRegistry());
-      initRemoteRegistration(functions, jarManager, remoteRegistry, remoteRegistry.getRetryAttempts());
+      initRemoteRegistration(functions, jarManager, remoteRegistry);
       jarManager.deleteQuietlyFromStagingArea();
 
       return DirectPlan.createDirectPlan(context, true,
@@ -154,43 +154,47 @@ public class CreateFunctionHandler extends DefaultSqlHandler {
    * Instantiates remote registration. First gets remote function registry with version.
    * Version is used to ensure that we update the same registry we validated against.
    * Then validates against list of remote jars.
-   * If validation is successful, starts updating remote function registry.
+   * If validation is successful, first copies jars to registry area and starts updating remote function registry.
    * If during update {@link VersionMismatchException} was detected,
-   * calls itself recursively to instantiate new remote registration process.
-   * Since remote registry version has changed, we need to re-validate against remote function registry one more time.
-   * Each time recursive call occurs, decreases retry attempts counter by one.
+   * attempts to repeat remote registration process till retry attempts exceeds the limit.
    * If retry attempts number hits 0, throws exception that failed to update remote function registry.
+   * In case of any error, if jars have been already copied to registry area, they will be deleted.
    *
    * @param functions list of functions present in jar
    * @param jarManager helper class for copying jars to registry area
    * @param remoteRegistry remote function registry
-   * @param retryAttempts number of retry attempts
    * @throws IOException in case of problems with copying jars to registry area
    */
   private void initRemoteRegistration(List<String> functions,
-                                      JarManager jarManager,
-                                      RemoteFunctionRegistry remoteRegistry,
-                                      int retryAttempts) throws IOException {
-    DataChangeVersion version = new DataChangeVersion();
-    List<Jar> remoteJars = remoteRegistry.getRegistry(version).getJarList();
-    validateAgainstRemoteRegistry(remoteJars, jarManager.getBinaryName(), functions);
-    jarManager.copyToRegistryArea();
-    boolean cleanUp = true;
-    List<Jar> jars = Lists.newArrayList(remoteJars);
-    jars.add(Jar.newBuilder().setName(jarManager.getBinaryName()).addAllFunctionSignature(functions).build());
-    Registry updatedRegistry = Registry.newBuilder().addAllJar(jars).build();
+                  JarManager jarManager,
+                  RemoteFunctionRegistry remoteRegistry) throws IOException {
+    int retryAttempts = remoteRegistry.getRetryAttempts();
+    boolean copyJars = true;
     try {
-      remoteRegistry.updateRegistry(updatedRegistry, version);
-      cleanUp = false;
-    } catch (VersionMismatchException ex) {
-      if (retryAttempts-- == 0) {
-        throw new DrillRuntimeException("Failed to update remote function registry. Exceeded retry attempts limit.");
+      while (retryAttempts >= 0) {
+        DataChangeVersion version = new DataChangeVersion();
+        List<Jar> remoteJars = remoteRegistry.getRegistry(version).getJarList();
+        validateAgainstRemoteRegistry(remoteJars, jarManager.getBinaryName(), functions);
+        if (copyJars) {
+          jarManager.copyToRegistryArea();
+          copyJars = false;
+        }
+        List<Jar> jars = Lists.newArrayList(remoteJars);
+        jars.add(Jar.newBuilder().setName(jarManager.getBinaryName()).addAllFunctionSignature(functions).build());
+        Registry updatedRegistry = Registry.newBuilder().addAllJar(jars).build();
+        try {
+          remoteRegistry.updateRegistry(updatedRegistry, version);
+          return;
+        } catch (VersionMismatchException ex) {
+          retryAttempts--;
+        }
       }
-      initRemoteRegistration(functions, jarManager, remoteRegistry, retryAttempts);
-    } finally {
-      if (cleanUp) {
+      throw new DrillRuntimeException("Failed to update remote function registry. Exceeded retry attempts limit.");
+    } catch (Exception e) {
+      if (!copyJars) {
         jarManager.deleteQuietlyFromRegistryArea();
       }
+      throw e;
     }
   }
 

http://git-wip-us.apache.org/repos/asf/drill/blob/fb32bfe2/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DropFunctionHandler.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DropFunctionHandler.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DropFunctionHandler.java
index 5269a4b..6e2801a 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DropFunctionHandler.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DropFunctionHandler.java
@@ -84,7 +84,7 @@ public class DropFunctionHandler extends DefaultSqlHandler {
         return DirectPlan.createDirectPlan(context, false, String.format("Jar with %s name is used. Action: %s", jarName, action));
       }
 
-      Jar deletedJar = unregister(jarName, remoteFunctionRegistry, remoteFunctionRegistry.getRetryAttempts());
+      Jar deletedJar = unregister(jarName, remoteFunctionRegistry);
       if (deletedJar == null) {
         return DirectPlan.createDirectPlan(context, false, String.format("Jar %s is not registered in remote registry", jarName));
       }
@@ -108,45 +108,44 @@ public class DropFunctionHandler extends DefaultSqlHandler {
   }
 
   /**
-   * First gets remote function registry with version.
+   * Gets remote function registry with version.
    * Version is used to ensure that we update the same registry we removed jars from.
    * Looks for a jar to be deleted, if founds one,
-   * attempts to update remote registry with updated list of jars, that excludes jar to be deleted.
+   * attempts to update remote registry with list of jars, that excludes jar to be deleted.
    * If during update {@link VersionMismatchException} was detected,
-   * calls itself recursively to instantiate new remote unregistration process.
-   * Since remote registry version has changed we need to look for jar to be deleted one more time.
-   * Each time recursive call occurs, decreases retry attempts counter by one.
+   * attempts to repeat unregistration process till retry attempts exceeds the limit.
    * If retry attempts number hits 0, throws exception that failed to update remote function registry.
    *
    * @param jarName jar name
    * @param remoteFunctionRegistry remote function registry
-   * @param retryAttempts number of retry attempts
    * @return jar that was unregistered, null otherwise
    */
-  private Jar unregister(String jarName, RemoteFunctionRegistry remoteFunctionRegistry, int retryAttempts) {
-    DataChangeVersion version = new DataChangeVersion();
-    Registry registry = remoteFunctionRegistry.getRegistry(version);
-    Jar jarToBeDeleted = null;
-    List<Jar> jars = Lists.newArrayList();
-    for (Jar j : registry.getJarList()) {
-      if (j.getName().equals(jarName)) {
-        jarToBeDeleted = j;
-      } else {
-        jars.add(j);
+  private Jar unregister(String jarName, RemoteFunctionRegistry remoteFunctionRegistry) {
+    int retryAttempts = remoteFunctionRegistry.getRetryAttempts();
+    while (retryAttempts >= 0) {
+      DataChangeVersion version = new DataChangeVersion();
+      Registry registry = remoteFunctionRegistry.getRegistry(version);
+      Jar jarToBeDeleted = null;
+      List<Jar> jars = Lists.newArrayList();
+      for (Jar j : registry.getJarList()) {
+        if (j.getName().equals(jarName)) {
+          jarToBeDeleted = j;
+        } else {
+          jars.add(j);
+        }
+      }
+      if (jarToBeDeleted == null) {
+        return null;
       }
-    }
-    if (jarToBeDeleted != null) {
       Registry updatedRegistry = Registry.newBuilder().addAllJar(jars).build();
       try {
         remoteFunctionRegistry.updateRegistry(updatedRegistry, version);
+        return jarToBeDeleted;
       } catch (VersionMismatchException ex) {
-        if (retryAttempts-- == 0) {
-          throw new DrillRuntimeException("Failed to update remote function registry. Exceeded retry attempts limit.");
-        }
-        unregister(jarName, remoteFunctionRegistry, retryAttempts);
+        retryAttempts--;
       }
     }
-    return jarToBeDeleted;
+    throw new DrillRuntimeException("Failed to update remote function registry. Exceeded retry attempts limit.");
   }
 
   /**

http://git-wip-us.apache.org/repos/asf/drill/blob/fb32bfe2/exec/java-exec/src/test/java/org/apache/drill/TestDynamicUDFSupport.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/test/java/org/apache/drill/TestDynamicUDFSupport.java b/exec/java-exec/src/test/java/org/apache/drill/TestDynamicUDFSupport.java
index 78cdbe2..10a03b7 100644
--- a/exec/java-exec/src/test/java/org/apache/drill/TestDynamicUDFSupport.java
+++ b/exec/java-exec/src/test/java/org/apache/drill/TestDynamicUDFSupport.java
@@ -177,7 +177,7 @@ public class TestDynamicUDFSupport extends BaseTestQuery {
   }
 
   @Test
-  public void testSuccessfulCreate() throws Exception {
+  public void testSuccessfulRegistration() throws Exception {
     copyDefaultJarsToStagingArea();
 
     String summary = "The following UDFs in jar %s have been registered:\n" +
@@ -272,6 +272,75 @@ public class TestDynamicUDFSupport extends BaseTestQuery {
   }
 
   @Test
+  public void testSuccessfulRegistrationAfterSeveralRetryAttempts() throws Exception {
+    RemoteFunctionRegistry remoteFunctionRegistry = spyRemoteFunctionRegistry();
+    copyDefaultJarsToStagingArea();
+
+    doThrow(new VersionMismatchException("Version mismatch detected", 1))
+            .doThrow(new VersionMismatchException("Version mismatch detected", 1))
+            .doCallRealMethod()
+            .when(remoteFunctionRegistry).updateRegistry(any(Registry.class), any(DataChangeVersion.class));
+
+    String summary = "The following UDFs in jar %s have been registered:\n" +
+            "[custom_lower(VARCHAR-REQUIRED)]";
+
+    testBuilder()
+            .sqlQuery("create function using jar '%s'", default_binary_name)
+            .unOrdered()
+            .baselineColumns("ok", "summary")
+            .baselineValues(true, String.format(summary, default_binary_name))
+            .go();
+
+    verify(remoteFunctionRegistry, times(3))
+            .updateRegistry(any(Registry.class), any(DataChangeVersion.class));
+
+    FileSystem fs = remoteFunctionRegistry.getFs();
+
+    assertFalse("Staging area should be empty", fs.listFiles(remoteFunctionRegistry.getStagingArea(), false).hasNext());
+    assertFalse("Temporary area should be empty", fs.listFiles(remoteFunctionRegistry.getTmpArea(), false).hasNext());
+
+    assertTrue("Binary should be present in registry area",
+            fs.exists(new Path(remoteFunctionRegistry.getRegistryArea(), default_binary_name)));
+    assertTrue("Source should be present in registry area",
+            fs.exists(new Path(remoteFunctionRegistry.getRegistryArea(), default_source_name)));
+
+    Registry registry = remoteFunctionRegistry.getRegistry();
+    assertEquals("Registry should contain one jar", registry.getJarList().size(), 1);
+    assertEquals(registry.getJar(0).getName(), default_binary_name);
+  }
+
+  @Test
+  public void testSuccessfulUnregistrationAfterSeveralRetryAttempts() throws Exception {
+    RemoteFunctionRegistry remoteFunctionRegistry = spyRemoteFunctionRegistry();
+    copyDefaultJarsToStagingArea();
+    test("create function using jar '%s'", default_binary_name);
+
+    reset(remoteFunctionRegistry);
+    doThrow(new VersionMismatchException("Version mismatch detected", 1))
+            .doThrow(new VersionMismatchException("Version mismatch detected", 1))
+            .doCallRealMethod()
+            .when(remoteFunctionRegistry).updateRegistry(any(Registry.class), any(DataChangeVersion.class));
+
+    String summary = "The following UDFs in jar %s have been unregistered:\n" +
+            "[custom_lower(VARCHAR-REQUIRED)]";
+
+    testBuilder()
+            .sqlQuery("drop function using jar '%s'", default_binary_name)
+            .unOrdered()
+            .baselineColumns("ok", "summary")
+            .baselineValues(true, String.format(summary, default_binary_name))
+            .go();
+
+    verify(remoteFunctionRegistry, times(3))
+            .updateRegistry(any(Registry.class), any(DataChangeVersion.class));
+
+    FileSystem fs = remoteFunctionRegistry.getFs();
+
+    assertFalse("Registry area should be empty", fs.listFiles(remoteFunctionRegistry.getRegistryArea(), false).hasNext());
+    assertEquals("Registry should be empty", remoteFunctionRegistry.getRegistry().getJarList().size(), 0);
+  }
+
+  @Test
   public void testExceedRetryAttemptsDuringRegistration() throws Exception {
     RemoteFunctionRegistry remoteFunctionRegistry = spyRemoteFunctionRegistry();
     copyDefaultJarsToStagingArea();
@@ -290,6 +359,18 @@ public class TestDynamicUDFSupport extends BaseTestQuery {
 
     verify(remoteFunctionRegistry, times(remoteFunctionRegistry.getRetryAttempts() + 1))
         .updateRegistry(any(Registry.class), any(DataChangeVersion.class));
+
+    FileSystem fs = remoteFunctionRegistry.getFs();
+
+    assertTrue("Binary should be present in staging area",
+            fs.exists(new Path(remoteFunctionRegistry.getStagingArea(), default_binary_name)));
+    assertTrue("Source should be present in staging area",
+            fs.exists(new Path(remoteFunctionRegistry.getStagingArea(), default_source_name)));
+
+    assertFalse("Registry area should be empty", fs.listFiles(remoteFunctionRegistry.getRegistryArea(), false).hasNext());
+    assertFalse("Temporary area should be empty", fs.listFiles(remoteFunctionRegistry.getTmpArea(), false).hasNext());
+
+    assertEquals("Registry should be empty", remoteFunctionRegistry.getRegistry().getJarList().size(), 0);
   }
 
   @Test
@@ -313,6 +394,17 @@ public class TestDynamicUDFSupport extends BaseTestQuery {
 
     verify(remoteFunctionRegistry, times(remoteFunctionRegistry.getRetryAttempts() + 1))
         .updateRegistry(any(Registry.class), any(DataChangeVersion.class));
+
+    FileSystem fs = remoteFunctionRegistry.getFs();
+
+    assertTrue("Binary should be present in registry area",
+            fs.exists(new Path(remoteFunctionRegistry.getRegistryArea(), default_binary_name)));
+    assertTrue("Source should be present in registry area",
+            fs.exists(new Path(remoteFunctionRegistry.getRegistryArea(), default_source_name)));
+
+    Registry registry = remoteFunctionRegistry.getRegistry();
+    assertEquals("Registry should contain one jar", registry.getJarList().size(), 1);
+    assertEquals(registry.getJar(0).getName(), default_binary_name);
   }
 
   @Test
@@ -445,7 +537,7 @@ public class TestDynamicUDFSupport extends BaseTestQuery {
   }
 
   @Test
-  public void testRegistryAreaCleanUpOnFail() throws Exception {
+  public void testRegistrationFailDuringRegistryUpdate() throws Exception {
     final RemoteFunctionRegistry remoteFunctionRegistry = spyRemoteFunctionRegistry();
     final FileSystem fs = remoteFunctionRegistry.getFs();
     final String errorMessage = "Failure during remote registry update.";