[GitHub] keith-turner closed pull request #453: Add retry mechanism to AccumuloReloadingVFSClassLoader.

[GitBox <gi...@apache.org>]

keith-turner closed pull request #453: Add retry mechanism to AccumuloReloadingVFSClassLoader.  
URL: https://github.com/apache/accumulo/pull/453
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/start/pom.xml b/start/pom.xml
index d14879efe6..4f87bb83fa 100644
--- a/start/pom.xml
+++ b/start/pom.xml
@@ -26,6 +26,10 @@
   <name>Apache Accumulo Start</name>
   <description>A library for launching Apache Accumulo services.</description>
   <dependencies>
+    <dependency>
+      <groupId>com.google.guava</groupId>
+      <artifactId>guava</artifactId>
+    </dependency>
     <dependency>
       <groupId>commons-io</groupId>
       <artifactId>commons-io</artifactId>
diff --git a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloReloadingVFSClassLoader.java b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloReloadingVFSClassLoader.java
index bdaa181f46..2a2c362705 100644
--- a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloReloadingVFSClassLoader.java
+++ b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloReloadingVFSClassLoader.java
@@ -36,6 +36,8 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import com.google.common.annotations.VisibleForTesting;
+
 /**
  * Classloader that delegates operations to a VFSClassLoader object. This class also listens for
  * changes in any of the files/directories that are in the classpath and will recreate the delegate
@@ -46,10 +48,16 @@
 
   private static final Logger log = LoggerFactory.getLogger(AccumuloReloadingVFSClassLoader.class);
 
-  // set to 5 mins. The rational behind this large time is to avoid a gazillion tservers all asking
+  // set to 5 mins. The rationale behind this large time is to avoid a gazillion tservers all asking
   // the name node for info too frequently.
   private static final int DEFAULT_TIMEOUT = 5 * 60 * 1000;
 
+  private volatile long maxWaitInterval = 60000;
+
+  private volatile long maxRetries = -1;
+
+  private volatile long sleepInterval = 5000;
+
   private FileObject[] files;
   private VFSClassLoader cl;
   private final ReloadingClassLoader parent;
@@ -80,6 +88,35 @@ public void run() {
           FileSystemManager vfs = AccumuloVFSClassLoader.generateVfs();
           FileObject[] files = AccumuloVFSClassLoader.resolve(vfs, uris);
 
+          long retries = 0;
+          long currentSleepMillis = sleepInterval;
+
+          if (files.length == 0) {
+            while (files.length == 0 && retryPermitted(retries)) {
+
+              try {
+                log.debug("VFS path was empty.  Waiting " + currentSleepMillis + " ms to retry");
+                Thread.sleep(currentSleepMillis);
+
+                files = AccumuloVFSClassLoader.resolve(vfs, uris);
+                retries++;
+
+                currentSleepMillis = Math.min(maxWaitInterval, currentSleepMillis + sleepInterval);
+
+              } catch (InterruptedException e) {
+                log.error("VFS Retry Interruped", e);
+                throw new RuntimeException(e);
+              }
+
+            }
+            // There is a chance that the listener was removed from the top level directory or
+            // its children if they were deleted within some time window. Re-add files to be
+            // monitored. The Monitor will ignore files that are already/still being monitored.
+            for (FileObject file : files) {
+              monitor.addFile(file);
+            }
+          }
+
           log.debug("Rebuilding dynamic classloader using files- {}", stringify(files));
 
           VFSClassLoader cl;
@@ -215,4 +252,20 @@ public String toString() {
     return buf.toString();
   }
 
+  @VisibleForTesting
+  void setMaxRetries(long maxRetries) {
+    this.maxRetries = maxRetries;
+  }
+
+  long getMaxRetries() {
+    return maxRetries;
+  }
+
+  long getMaxWaitInterval() {
+    return maxWaitInterval;
+  }
+
+  private boolean retryPermitted(long retries) {
+    return (maxRetries < 0 || retries < maxRetries);
+  }
 }
diff --git a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java
index 270cd10c2b..48b19e814e 100644
--- a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java
+++ b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java
@@ -152,14 +152,19 @@ public void run() {
         case IMAGINARY:
           // assume its a pattern
           String pattern = fo.getName().getBaseName();
-          if (fo.getParent() != null && fo.getParent().getType() == FileType.FOLDER) {
+          if (fo.getParent() != null) {
+            // still monitor the parent
             pathsToMonitor.add(fo.getParent());
-            FileObject[] children = fo.getParent().getChildren();
-            for (FileObject child : children) {
-              if (child.getType() == FileType.FILE
-                  && child.getName().getBaseName().matches(pattern)) {
-                classpath.add(child);
+            if (fo.getParent().getType() == FileType.FOLDER) {
+              FileObject[] children = fo.getParent().getChildren();
+              for (FileObject child : children) {
+                if (child.getType() == FileType.FILE
+                    && child.getName().getBaseName().matches(pattern)) {
+                  classpath.add(child);
+                }
               }
+            } else {
+              log.debug("classpath entry " + fo.getParent() + " is " + fo.getParent().getType());
             }
           } else {
             log.warn("ignoring classpath entry {}", fo);
diff --git a/start/src/test/java/org/apache/accumulo/start/classloader/vfs/AccumuloReloadingVFSClassLoaderTest.java b/start/src/test/java/org/apache/accumulo/start/classloader/vfs/AccumuloReloadingVFSClassLoaderTest.java
index adf7213b30..a052412bc6 100644
--- a/start/src/test/java/org/apache/accumulo/start/classloader/vfs/AccumuloReloadingVFSClassLoaderTest.java
+++ b/start/src/test/java/org/apache/accumulo/start/classloader/vfs/AccumuloReloadingVFSClassLoaderTest.java
@@ -103,6 +103,9 @@ public ClassLoader getClassLoader() {
     FileObject[] files = ((VFSClassLoader) arvcl.getClassLoader()).getFileObjects();
     Assert.assertArrayEquals(createFileSystems(dirContents), files);
 
+    // set retry settings sufficiently low that not everything is reloaded in the first round
+    arvcl.setMaxRetries(1);
+
     Class<?> clazz1 = arvcl.getClassLoader().loadClass("test.HelloWorld");
     Object o1 = clazz1.newInstance();
     Assert.assertEquals("Hello World!", o1.toString());
@@ -135,6 +138,57 @@ public ClassLoader getClassLoader() {
     arvcl.close();
   }
 
+  @Test
+  public void testReloadingWithLongerTimeout() throws Exception {
+    FileObject testDir = vfs.resolveFile(folder1.getRoot().toURI().toString());
+    FileObject[] dirContents = testDir.getChildren();
+
+    AccumuloReloadingVFSClassLoader arvcl = new AccumuloReloadingVFSClassLoader(folderPath, vfs,
+        new ReloadingClassLoader() {
+          @Override
+          public ClassLoader getClassLoader() {
+            return ClassLoader.getSystemClassLoader();
+          }
+        }, 1000, true);
+
+    FileObject[] files = ((VFSClassLoader) arvcl.getClassLoader()).getFileObjects();
+    Assert.assertArrayEquals(createFileSystems(dirContents), files);
+
+    // set retry settings sufficiently high such that reloading happens in the first rounds
+    arvcl.setMaxRetries(3);
+
+    Class<?> clazz1 = arvcl.getClassLoader().loadClass("test.HelloWorld");
+    Object o1 = clazz1.newInstance();
+    Assert.assertEquals("Hello World!", o1.toString());
+
+    // Check that the class is the same before the update
+    Class<?> clazz1_5 = arvcl.getClassLoader().loadClass("test.HelloWorld");
+    Assert.assertEquals(clazz1, clazz1_5);
+
+    assertTrue(new File(folder1.getRoot(), "HelloWorld.jar").delete());
+
+    // VFS-487 significantly wait to avoid failure
+    Thread.sleep(7000);
+
+    // Update the class
+    FileUtils.copyURLToFile(this.getClass().getResource("/HelloWorld.jar"),
+        folder1.newFile("HelloWorld2.jar"));
+
+    // Wait for the monitor to notice
+    // VFS-487 significantly wait to avoid failure
+    Thread.sleep(7000);
+
+    Class<?> clazz2 = arvcl.getClassLoader().loadClass("test.HelloWorld");
+    Object o2 = clazz2.newInstance();
+    Assert.assertEquals("Hello World!", o2.toString());
+
+    // This is true because they are loaded by the same classloader due to the new retry
+    Assert.assertTrue(clazz1.equals(clazz2));
+    Assert.assertFalse(o1.equals(o2));
+
+    arvcl.close();
+  }
+
   // This test fails because of an error with the underlying monitor (ACCUMULO-1507/VFS-487).
   // Uncomment when this has been addressed.
   //


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services