[GitHub] [geode] bschuchardt commented on a change in pull request #5763: GEODE-8521: detect if a p2p reader thread is hung

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

bschuchardt commented on a change in pull request #5763:
URL: https://github.com/apache/geode/pull/5763#discussion_r527801127



##########
File path: geode-core/src/main/java/org/apache/geode/internal/monitoring/ThreadsMonitoringImpl.java
##########
@@ -96,39 +98,60 @@ public void updateThreadStatus() {
 
   @Override
   public boolean startMonitor(Mode mode) {
-    AbstractExecutor absExtgroup;
+    return startMonitoring(createAbstractExecutor(mode));
+  }
+
+  @Override
+  public void endMonitor() {
+    this.monitorMap.remove(Thread.currentThread().getId());
+  }
+
+  @VisibleForTesting
+  boolean isMonitoring() {
+    return monitorMap.containsKey(Thread.currentThread().getId());
+  }
+
+  @Override
+  public AbstractExecutor createAbstractExecutor(Mode mode) {
     switch (mode) {
       case FunctionExecutor:
-        absExtgroup = new FunctionExecutionPooledExecutorGroup(this);
-        break;
+        return new FunctionExecutionPooledExecutorGroup();
       case PooledExecutor:
-        absExtgroup = new PooledExecutorGroup(this);
-        break;
+        return new PooledExecutorGroup();
       case SerialQueuedExecutor:
-        absExtgroup = new SerialQueuedExecutorGroup(this);
-        break;
+        return new SerialQueuedExecutorGroup();
       case OneTaskOnlyExecutor:
-        absExtgroup = new OneTaskOnlyExecutorGroup(this);
-        break;
+        return new OneTaskOnlyExecutorGroup();
       case ScheduledThreadExecutor:
-        absExtgroup = new ScheduledThreadPoolExecutorWKAGroup(this);
-        break;
+        return new ScheduledThreadPoolExecutorWKAGroup();
       case AGSExecutor:
-        absExtgroup = new GatewaySenderEventProcessorGroup(this);
-        break;
+        return new GatewaySenderEventProcessorGroup();
+      case P2PReaderExecutor:
+        return new P2PReaderExecutorGroup();
       default:
-        return false;
+        throw new IllegalStateException("Unhandled mode=" + mode);
     }
-    this.monitorMap.put(Thread.currentThread().getId(), absExtgroup);
+  }
+
+  @Override
+  public boolean startMonitoring(AbstractExecutor executor) {
+    executor.setStartTime(System.currentTimeMillis());

Review comment:
       I'm a little concerned about the impact of taking yet another millisecond clock reading on every message that we dispatch.  This should be run through extensive performance testing before merging it to develop.




----------------------------------------------------------------
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.

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