In JvmPauseMonitor monitorThread gets initialized only in RMActiveServices#serviceStart but in testcases we are not starting the same.

One fix could be JvmPauseMonitor#stop

  public void stop() {
    shouldRun = false;
    if (null != monitorThread) {
      monitorThread.interrupt();
      try {
        monitorThread.join();
      } catch (InterruptedException e) {
        Thread.currentThread().interrupt();
      }
    }
  }

Thoughts?

-1 overall

This message was automatically generated.

thanks of the patch!!
One comment, the below code modification is not required right

+    if (shouldRun) {
+      monitorThread.start();
+    }

Thanks Gabor Liptak for working out a patch and Rohith Sharma K S for the comments!
Instead of adding "if (shouldRun)" to start monitorThread in start(), I think the right thing to do is set "shouldRun = true" in start(), so JvmPauseMonitor could be restarted after stop (or it cannot be restart again).
In addition, given the patch all happens in hadoop-common-project, I think we should move the JIRA from YARN to HADOOP.

Rohith Sharma K S That is my attempt to make this "state-friendly" ... If stop() happens before start completed, you would leave a thread behind.

Junping Du The "proper way" would be to synchronize both start()/stop() (in particular if restart is an expectation). Would you like an updated patch implementing?

given the patch all happens in hadoop-common-project, I think we should move the JIRA from YARN to HADOOP.

make sense, this fix is in Hadoop common, moving to HADOOP common project.

Assigned to me by mistake, assigned back to Gabor Liptak

The "proper way" would be to synchronize both start()/stop() (in particular if restart is an expectation). Would you like an updated patch implementing?

I am not sure if restart is an expectation at the beginning. But theoretically, any services should be restartable. However, just double checked with other services, like: NodeManager, ResourceManager, etc., it seems like all these services are implemented as one time only service. So I am fine with keeping things like this to conform the same pattern with other services. I will file another separated JIRA to discuss restartable issue unless someone tells me a solid reason on this.
Based on above comments, we don't need to handle start() after stop() case, so "if (shouldRun)

" is not necessary or not sufficient (at least we should log some warn messages). Either way should be fine to me.

Junping Du I suggest keeping the if (shouldRun), as it helps in the edge case corrected here (when stop() comes before start() completes during unit tests).

I suggest keeping the if (shouldRun), as it helps in the edge case corrected here (when stop() comes before start() completes during unit tests)

If so, please add a warn message or user have not hints if it get started or not.

Added warn message.

Latest patch LGTM. +1 pending on Jenkins result.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

There are seems to be problem with Jenkins test against Hadoop Common Project. Filed INFRA-10111 to track that.

Gabor Liptak, this patch got conflicted with YARN-3999 which just got committed. would you mind rebasing on that.

Uploaded rebased patch.

-1 overall

This message was automatically generated.

why not make JvmPauseMonitor a subclass of ActiveService, so getting the lifecycle and non-reentrant serviceStop calls? It'd also make it trivial to deploy under any instance of CompositeService, so avoid needing any specific hooks in any service class planning to use it.

I believe I created a JIRA long time ago to allow for pause/resume or stop/start semantics for services. Any takers?

I created HADOOP-12320 yesterday (which might be a duplicate as per Karthik Kambatla

Steve Loughran Would you see this being worked under this Jira or the new "improvement" one?

Let's fix the NPE in this JIRA, use HADOOP-12321 to make JvmPauseMonitor a Service, and use HADOOP-12320 to discuss making Services restartable in general.

+1. Committing.

I've committed the patch to trunk and branch-2. Thanks Gabor Liptak for the contribution.

FAILURE: Integrated in Hadoop-trunk-Commit #8858 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8858/)
HADOOP-12313. NPE in JvmPauseMonitor when calling stop() before start(). (wheat9: rev 817ae221ac1ca8552748f6310d29ea1b26b5a21b)

• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JvmPauseMonitor.java
• hadoop-common-project/hadoop-common/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #701 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/701/)
HADOOP-12313. NPE in JvmPauseMonitor when calling stop() before start(). (wheat9: rev 817ae221ac1ca8552748f6310d29ea1b26b5a21b)

• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JvmPauseMonitor.java
• hadoop-common-project/hadoop-common/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2642 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2642/)
HADOOP-12313. NPE in JvmPauseMonitor when calling stop() before start(). (wheat9: rev 817ae221ac1ca8552748f6310d29ea1b26b5a21b)

• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JvmPauseMonitor.java
• hadoop-common-project/hadoop-common/CHANGES.txt

-1 overall

This message was automatically generated.

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #712 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/712/)
HADOOP-12313. NPE in JvmPauseMonitor when calling stop() before start(). (wheat9: rev 817ae221ac1ca8552748f6310d29ea1b26b5a21b)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JvmPauseMonitor.java

FAILURE: Integrated in Hadoop-Yarn-trunk #1436 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1436/)
HADOOP-12313. NPE in JvmPauseMonitor when calling stop() before start(). (wheat9: rev 817ae221ac1ca8552748f6310d29ea1b26b5a21b)

• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JvmPauseMonitor.java
• hadoop-common-project/hadoop-common/CHANGES.txt

although its nice to have a fix, I'm testing HADOOP-12321 for a broader one ... it relies on the yarn service model for a strict state machine & won't won't do things like permit calls to start twice spawning off a thread again, etc.

ABORTED: Integrated in Hadoop-Hdfs-trunk-Java8 #630 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/630/)
HADOOP-12313. NPE in JvmPauseMonitor when calling stop() before start(). (wheat9: rev 817ae221ac1ca8552748f6310d29ea1b26b5a21b)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JvmPauseMonitor.java

FAILURE: Integrated in Hadoop-Hdfs-trunk #2569 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2569/)
HADOOP-12313. NPE in JvmPauseMonitor when calling stop() before start(). (wheat9: rev 817ae221ac1ca8552748f6310d29ea1b26b5a21b)

• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JvmPauseMonitor.java
• hadoop-common-project/hadoop-common/CHANGES.txt