YARN-6061. Add a customized uncaughtexceptionhandler for critical threads #182

flyrain · 2017-01-20T00:02:59Z

No description provided.

…eads in RM.

kambatla · 2017-01-30T22:31:01Z

.../org/apache/hadoop/yarn/server/resourcemanager/RMCriticalThreadUncaughtExceptionHandler.java

+
+  @Override
+  public void uncaughtException(Thread t, Throwable e) {
+    LOG.fatal("Critical thread " + t + " crashed!", e);


Should we log t.getName() instead of t itself?

kambatla · 2017-01-30T22:31:42Z

.../org/apache/hadoop/yarn/server/resourcemanager/RMCriticalThreadUncaughtExceptionHandler.java

+              new Exception(e)));
+    }
+  }
+}


New line at the end of the file?

kambatla · 2017-01-30T22:33:19Z

.../org/apache/hadoop/yarn/server/resourcemanager/RMCriticalThreadUncaughtExceptionHandler.java

+      new Thread() {
+        @Override
+        public void run() {
+          rmContext.getResourceManager().handleTransitionToStandBy();


Are we sure we can call this synchronously? Would it be more appropriate to do the transition to standby in a different thread, like in RMStateStore? If yes, may be we could update RMStateStore to use this too?

My bad. I see this is being done in a new thread.

That said, may be, we could consider abstracting this out so this handler and the RMStateStore just call that abstraction?

kambatla · 2017-01-30T22:34:02Z

.../apache/hadoop/yarn/server/resourcemanager/TestRMCriticalThreadUncaughtExceptionHandler.java

+import static org.mockito.Mockito.verify;
+
+/**
+ * This class is to test {@link RMCriticalThreadUncaughtExceptionHandler}.


Not sure if this javadoc is required. This is a convention we generally follow.

kambatla · 2017-01-30T23:12:13Z

.../apache/hadoop/yarn/server/resourcemanager/TestRMCriticalThreadUncaughtExceptionHandler.java

+    ExitUtil.disableSystemExit();
+
+    // Create a MockRM and start it
+    ResourceManager resourceManager = new MockRM();


If we are anyways creating the MockRM, why not start it and have some thread throw an uncaught exception and verify the RM behavior in two cases (two tests?) - exit and transition to standby?

Add one unit test to check if RM transition to standby.

…eads in RM.

kambatla · 2017-02-01T00:51:15Z

...urcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ResourceManager.java

+   * Transition to standby in a new thread.
+   */
+  public void handleTransitionToStandByInNewThread() {
+    new Thread() {


Instead of using an anonymous class, can we define this as a separate Thread and name it for easier debugging?

kambatla · 2017-02-01T00:53:31Z

...doop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/TestRMFailover.java

+   * @throws InterruptedException if any
+   */
+  @Test
+  public void testUncaughtExceptionHandlerWithHAEnabled()


Nice tests!

…eads in RM.

kambatla · 2017-02-01T17:00:35Z

...urcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ResourceManager.java

-          } catch (Exception e) {
-            LOG.fatal("Failed to transition RM to Standby mode.", e);
-            ExitUtil.terminate(1, e);
+    Thread standByTransitionThread = new Thread(new StandByTransitionThread());


Sorry for not identifying this earlier. We should make this thread-safe in case this is triggered by two critical threads failing at the same time.

Also, would it make sense to create an instance of the Runnable on transition to active, and start a new thread on a need-to basis. If all threads use a single instance of the Runnable, may be it is easier to coordinate?

kambatla · 2017-02-01T17:03:21Z

...urcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ResourceManager.java

+    standByTransitionThread.start();
+  }
+
+  private class StandByTransitionThread implements Runnable {


Naming the Runnable a Thread sounds confusing. Can we change it to TransitionToStandbyRunnable or some such?

…eads in RM.

kambatla

Patch looks good. Minor nits on documentation before we do the final push.

kambatla · 2017-02-07T22:39:24Z

.../org/apache/hadoop/yarn/server/resourcemanager/RMCriticalThreadUncaughtExceptionHandler.java

+import org.apache.hadoop.yarn.conf.HAUtil;
+
+/**
+ * This class either shutdowns {@link ResourceManager} or makes


s/shutdowns/shuts down

s/makes RM transition/ transitions the RM

s/if any uncaught exception.../if a critical thread throws an uncaught exception.

kambatla · 2017-02-07T22:43:43Z

...urcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ResourceManager.java

+    standByTransitionThread.start();
+  }
+
+  private class StandByTransitionRunnable implements Runnable {


Let us add javadoc for this class, and include details on how we use the same runnable.

kambatla · 2017-02-07T22:44:48Z

...urcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ResourceManager.java

+
+    @Override
+    public void run() {
+      // Prevent from running again if it has run.


Add more detail here: "Run this only once, even if multiple threads end up triggering this simultaneously."

kambatla · 2017-02-07T22:45:10Z

...urcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ResourceManager.java

+  }
+
+  private class StandByTransitionRunnable implements Runnable {
+    private AtomicBoolean hasRun = new AtomicBoolean(false);


Maybe, rename this to hasAlreadyRun? And, again add some javadoc here too?

…eads in RM.

kambatla · 2017-02-10T04:02:50Z

.../org/apache/hadoop/yarn/server/resourcemanager/RMCriticalThreadUncaughtExceptionHandler.java

+ * {@code setUncaughtExceptionHandler(Thread.UncaughtExceptionHandler)}
+ * in the thread entry point or after creation of threads.
+ */
+@Public


We don't need to expose this outside YARN at all. This should be @Private. Let us remove @evolving altogether.

kambatla

Patch looks good to go, but for the one nit on Audience and Stability annotations.

…eads in RM.

flyrain · 2017-04-04T16:44:53Z

Committed

…erge as input Also updated documentation for join and partitionBy. Author: Prateek Maheshwari <[email protected]> Reviewers: Jacob Maes <[email protected]> Closes apache#182 from prateekm/documentation-cleanup

YARN-6061. Add a customized uncaughtexceptionhandler for critical thr…

ce2b20a

…eads in RM.

flyrain force-pushed the yarn-6061 branch from fbbc209 to ce2b20a Compare January 30, 2017 09:52

kambatla reviewed Jan 30, 2017

View reviewed changes

YARN-6061. Add a customized uncaughtexceptionhandler for critical thr…

c053b99

…eads in RM.

kambatla reviewed Feb 1, 2017

View reviewed changes

YARN-6061. Add a customized uncaughtexceptionhandler for critical thr…

db29207

…eads in RM.

kambatla reviewed Feb 1, 2017

View reviewed changes

flyrain added 2 commits February 1, 2017 17:17

YARN-6061. Add a customized uncaughtexceptionhandler for critical thr…

1abba7d

…eads in RM.

YARN-6061. Add a customized uncaughtexceptionhandler for critical thr…

c9d2057

…eads in RM.

kambatla reviewed Feb 7, 2017

View reviewed changes

YARN-6061. Add a customized uncaughtexceptionhandler for critical thr…

cd06410

…eads in RM.

flyrain force-pushed the yarn-6061 branch from 9297429 to cd06410 Compare February 8, 2017 19:35

kambatla reviewed Feb 10, 2017

View reviewed changes

YARN-6061. Add a customized uncaughtexceptionhandler for critical thr…

5d36c5f

…eads in RM.

kambatla approved these changes Feb 12, 2017

View reviewed changes

YARN-6061. Add a customized uncaughtexceptionhandler for critical thr…

8b3da60

…eads in RM.

flyrain closed this Apr 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

YARN-6061. Add a customized uncaughtexceptionhandler for critical threads #182

YARN-6061. Add a customized uncaughtexceptionhandler for critical threads #182

flyrain commented Jan 20, 2017

kambatla Jan 30, 2017

kambatla Jan 30, 2017

kambatla Jan 30, 2017

kambatla Jan 31, 2017

kambatla Jan 30, 2017

kambatla Jan 30, 2017

flyrain Jan 31, 2017

kambatla Feb 1, 2017

kambatla Feb 1, 2017

kambatla Feb 1, 2017

kambatla Feb 1, 2017

kambatla Feb 1, 2017

kambatla left a comment

kambatla Feb 7, 2017

kambatla Feb 7, 2017

kambatla Feb 7, 2017

kambatla Feb 7, 2017

kambatla Feb 10, 2017

kambatla left a comment

flyrain commented Apr 4, 2017

YARN-6061. Add a customized uncaughtexceptionhandler for critical threads #182

YARN-6061. Add a customized uncaughtexceptionhandler for critical threads #182

Conversation

flyrain commented Jan 20, 2017

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

kambatla 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

kambatla left a comment

Choose a reason for hiding this comment

flyrain commented Apr 4, 2017