accumulo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ACCUMULO-1292) Tablet constructor can hang on vfs classloader, preventing tablets from loading
Date Thu, 22 Jan 2015 18:04:35 GMT

    [ https://issues.apache.org/jira/browse/ACCUMULO-1292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14287867#comment-14287867
] 

Josh Elser commented on ACCUMULO-1292:
--------------------------------------

Thanks for the details last night, Dave. I think I know where my confusion was coming from
now. You're trying to avoid blocking all threads from executing knowing that the classloader
needs to be refreshed at the cost of continuing to run with "stale" artifacts? That's why
you wanted to use the async thread.

Assuming I'm on the same page now, that makes a lot more sense. What would happen with the
VFS classloader if a filesystem change happened (Jar was replaced), refresh on the classloader
is started but takes a long time, class load is requested for a class that was from the replaced
jar? Is that safe -- it would continue to load the old version of the class? Would requests
before the classloader is updated fail?

{code}
+  private final ThreadPoolExecutor executor =
+      new ThreadPoolExecutor(1, 1, 1, SECONDS, new ArrayBlockingQueue<Runnable>(2));
{code}

Wouldn't {{Executors.newSingleThreadExecutor()}} be more concise? Is your keepAliveTime actually
doing to do anything with a coreSize of 1?

{code}
+  private void scheduleRefresh() {
+    try {
+      executor.execute(new Runnable() {
+        @Override
+        public void run() {
+          try {
+            recreateClassloaderNow();
+          } catch (FileSystemException e) {
+            log.error("Error refreshing classloader", e);
+            clRef.set(null);
+          }
+        }
+      });
+    } catch (RejectedExecutionException e) {
+      // don't care, queue is full
+    }
+  }
{code}

Make the Runnable once. You don't need to make a new instance of it every time you call this
method. Also, debug or trace level logging in the catch?

{code}
   public void close() {
+    executor.shutdown();
     monitor.stop();
   }
{code}

This will initiate the shutdown, but doesn't wait for it to finish. If the tserver is trying
to stop but is stuck trying to reload a class, we don't want it to block the tserver from
exiting (especially if the remote IO eats the interrupted exception). Need to make sure that
the async refreshing thread is a daemon or provide a way to stop the thread.

I see you made some modifications to AccumuloVFSClassLoaderTest as well, but it doesn't look
like those are actually testing anything related to these changes. Have you put any thought
into how you can verify the correctness of these changes?

> Tablet constructor can hang on vfs classloader, preventing tablets from loading
> -------------------------------------------------------------------------------
>
>                 Key: ACCUMULO-1292
>                 URL: https://issues.apache.org/jira/browse/ACCUMULO-1292
>             Project: Accumulo
>          Issue Type: Bug
>          Components: tserver
>    Affects Versions: 1.5.0, 1.6.0, 1.6.1
>            Reporter: John Vines
>            Assignee: Eric Newton
>             Fix For: 1.7.0, 1.6.3
>
>         Attachments: ACCUMULO-1292-atomic-update.patch, ACCUMULO-1292-using-locks.patch,
ACCUMULO-1292.patch
>
>
> Taken from TODO from r1424106 regarding ACCUMULO-867. This is something that we should
at least look into more before 1.5 is released.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message