[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #15155: [Broker] Make health check fail if dead locked threads are detected

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

michaeljmarshall commented on code in PR #15155:
URL: https://github.com/apache/pulsar/pull/15155#discussion_r849620418


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java:
##########
@@ -335,6 +346,27 @@ public void healthCheck(@Suspended AsyncResponse asyncResponse,
                 });
     }
 
+    private void checkDeadlockedThreads() {
+        ThreadMXBean threadBean = ManagementFactory.getThreadMXBean();
+        long[] threadIds = threadBean.findDeadlockedThreads();
+        if (threadIds != null && threadIds.length > 0) {
+            ThreadInfo[] threadInfos = threadBean.getThreadInfo(threadIds, false, false);
+            String threadNames = Arrays.stream(threadInfos)
+                    .map(threadInfo -> threadInfo.getThreadName() + "(tid=" + threadInfo.getThreadId() + ")").collect(
+                            Collectors.joining(", "));
+            if (System.currentTimeMillis() - threadDumpLoggedTimestamp
+                    > LOG_THREADDUMP_INTERVAL_WHEN_DEADLOCK_DETECTED) {
+                threadDumpLoggedTimestamp = System.currentTimeMillis();
+                LOG.error("Deadlocked threads detected. {}\n{}", threadNames,
+                        ThreadDumpUtil.buildThreadDiagnosticString());
+            } else {
+                LOG.error("Deadlocked threads detected. {}", threadNames);
+            }
+            throw new IllegalStateException("Deadlocked threads detected. " + threadNames);

Review Comment:
   Can you explain why/how you categorize deadlock as a small problem?
   
   Deadlock is an unrecoverable state for threads on the JVM, and the presence of deadlocked threads means something is wrong/stuck within the broker. I agree with @lhotari that we should consider deadlocked brokers to be unhealthy.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java:
##########
@@ -335,6 +346,27 @@ public void healthCheck(@Suspended AsyncResponse asyncResponse,
                 });
     }
 
+    private void checkDeadlockedThreads() {

Review Comment:
   A unit test would be nice to add, but if it's hard to implement, I think we should still merge this PR.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org