GEODE-9478: Fix status --dir to use file controller

- Previously when you only specified --dir option the PID was read from the member workDir and the status request was attempted to solved by using the attachment API, and after that JMX interface. But given only --dir was specified the controller resolving the request should be FileProcessController instead. - Logic has been changed for both servers and locators to always use FileProcessConroller whenever only --dir flag is specified. - Added an UT to verify new code. - Modified several ITs to verify the new behaviour. - Deleted the following ITs which no longer apply with the new logic: * statusWithEmptyPidFileThrowsIllegalArgumentException * statusWithEmptyWorkingDirectoryReturnsNotRespondingWithDetails * statusWithStalePidFileReturnsNotResponding
apache · Aug 9, 2021 · 0371b8f · 0371b8f
1 parent 32e5a5f
commit 0371b8f
Show file tree

Hide file tree

Showing 10 changed files with 107 additions and 257 deletions.
diff --git a/...ntegrationTest/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java b/...ntegrationTest/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java
@@ -14,8 +14,6 @@
  */
 package org.apache.geode.distributed;
 
-import static java.lang.management.ManagementFactory.getRuntimeMXBean;
-import static org.apache.geode.distributed.AbstractLauncher.Status.NOT_RESPONDING;
 import static org.apache.geode.distributed.AbstractLauncher.Status.ONLINE;
 import static org.apache.geode.distributed.AbstractLauncher.Status.STOPPED;
 import static org.apache.geode.distributed.ConfigurationProperties.DISABLE_AUTO_RECONNECT;
@@ -195,6 +193,13 @@ public void statusWithPidReturnsOnlineWithDetails() throws Exception {
   public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws Exception {
     givenRunningLocator();
 
+    // Keep it as we are going to overwrite it
+    int pid = readPidFile();
+
+    // Sets a fake PID to ensure that the request is going through FileProcessController,
+    // because if not, the request would fail due to the PID not existing
+    givenPidFile(fakePid);
+
     LocatorState locatorState = new Builder()
         .setWorkingDirectory(getWorkingDirectoryPath())
         .build()
@@ -208,62 +213,11 @@ public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws Exceptio
     assertThat(locatorState.getJvmArguments()).isEqualTo(getJvmArguments());
     assertThat(locatorState.getLogFile()).isEqualTo(getLogFilePath());
     assertThat(locatorState.getMemberName()).isEqualTo(getUniqueName());
-    assertThat(locatorState.getPid().intValue()).isEqualTo(readPidFile());
+    assertThat(locatorState.getPid().intValue()).isEqualTo(pid);
     assertThat(locatorState.getUptime()).isGreaterThan(0);
     assertThat(locatorState.getWorkingDirectory()).isEqualTo(new File(".").getCanonicalPath());
   }
 
-  @Test
-  public void statusWithEmptyPidFileThrowsIllegalArgumentException() {
-    givenEmptyPidFile();
-    LocatorLauncher launcher = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build();
-
-    Throwable thrown = catchThrowable(() -> launcher.status());
-
-    assertThat(thrown)
-        .isInstanceOf(IllegalArgumentException.class)
-        .hasMessageContaining("Invalid pid 'null' found in");
-  }
-
-  @Test
-  public void statusWithEmptyWorkingDirectoryReturnsNotRespondingWithDetails() throws Exception {
-    givenEmptyWorkingDirectory();
-
-    LocatorState locatorState = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build()
-        .status();
-
-    assertThat(locatorState.getStatus()).isEqualTo(NOT_RESPONDING);
-    assertThat(locatorState.getClasspath()).isNull();
-    assertThat(locatorState.getGemFireVersion()).isEqualTo(GemFireVersion.getGemFireVersion());
-    assertThat(locatorState.getHost()).isEqualTo(getLocalHost().getCanonicalHostName());
-    assertThat(locatorState.getJavaVersion()).isEqualTo(System.getProperty("java.version"));
-    assertThat(locatorState.getJvmArguments()).isEqualTo(getRuntimeMXBean().getInputArguments());
-    assertThat(locatorState.getLogFile()).isNull();
-    assertThat(locatorState.getMemberName()).isNull();
-    assertThat(locatorState.getPid()).isNull();
-    assertThat(locatorState.getUptime().intValue()).isEqualTo(0);
-    assertThat(locatorState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath());
-  }
-
-  /**
-   * This test takes > 1 minute to run in {@link LocatorLauncherLocalFileIntegrationTest}.
-   */
-  @Test
-  public void statusWithStalePidFileReturnsNotResponding() {
-    givenPidFile(fakePid);
-
-    LocatorState locatorState = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build()
-        .status();
-
-    assertThat(locatorState.getStatus()).isEqualTo(NOT_RESPONDING);
-  }
-
   @Test
   public void stopWithPidReturnsStopped() {
     givenRunningLocator();

diff --git a/...tegrationTest/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java b/...tegrationTest/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java
@@ -14,12 +14,10 @@
  */
 package org.apache.geode.distributed;
 
-import static org.apache.geode.distributed.AbstractLauncher.Status.NOT_RESPONDING;
 import static org.apache.geode.distributed.AbstractLauncher.Status.ONLINE;
 import static org.apache.geode.distributed.AbstractLauncher.Status.STOPPED;
 import static org.apache.geode.internal.inet.LocalHostUtil.getLocalHost;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.catchThrowable;
 
 import java.io.File;
 import java.net.BindException;
@@ -149,6 +147,13 @@ public void statusWithPidReturnsOnlineWithDetails() throws Exception {
   public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws Exception {
     givenRunningLocator(withPort(0));
 
+    // Keep it as we are going to overwrite it
+    int pid = readPidFile();
+
+    // Sets a fake PID to ensure that the request is going through FileProcessController,
+    // because if not, the request would fail due to the PID not existing
+    givenPidFile(fakePid);
+
     LocatorState locatorState = new Builder()
         .setWorkingDirectory(getWorkingDirectoryPath())
         .build()
@@ -161,62 +166,12 @@ public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws Exceptio
     assertThat(locatorState.getJvmArguments()).isEqualTo(getJvmArguments());
     assertThat(locatorState.getLogFile()).isEqualTo(getLogFile().getCanonicalPath());
     assertThat(locatorState.getMemberName()).isEqualTo(getUniqueName());
-    assertThat(locatorState.getPid().intValue()).isEqualTo(readPidFile());
+    assertThat(locatorState.getPid().intValue()).isEqualTo(pid);
     assertThat(locatorState.getStatus()).isEqualTo(ONLINE);
     assertThat(locatorState.getUptime()).isGreaterThan(0);
     assertThat(locatorState.getWorkingDirectory()).isEqualToIgnoringCase(getWorkingDirectoryPath());
   }
 
-  @Test
-  public void statusWithEmptyPidFileThrowsIllegalArgumentException() {
-    givenEmptyPidFile();
-    LocatorLauncher launcher = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build();
-
-    Throwable thrown = catchThrowable(() -> launcher.status());
-
-    assertThat(thrown)
-        .isInstanceOf(IllegalArgumentException.class)
-        .hasMessageContaining("Invalid pid 'null' found in");
-  }
-
-  @Test
-  public void statusWithEmptyWorkingDirectoryReturnsNotRespondingWithDetails() throws Exception {
-    givenEmptyWorkingDirectory();
-
-    LocatorState locatorState = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build()
-        .status();
-
-    assertThat(locatorState.getClasspath()).isNull();
-    assertThat(locatorState.getGemFireVersion()).isEqualTo(GemFireVersion.getGemFireVersion());
-    assertThat(locatorState.getHost()).isEqualTo(getLocalHost().getCanonicalHostName());
-    assertThat(locatorState.getJavaVersion()).isEqualTo(System.getProperty("java.version"));
-    assertThat(locatorState.getLogFile()).isNull();
-    assertThat(locatorState.getMemberName()).isNull();
-    assertThat(locatorState.getPid()).isNull();
-    assertThat(locatorState.getStatus()).isEqualTo(NOT_RESPONDING);
-    assertThat(locatorState.getUptime().intValue()).isEqualTo(0);
-    assertThat(locatorState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath());
-  }
-
-  /**
-   * This test takes > 1 minute to run in {@link LocatorLauncherRemoteFileIntegrationTest}.
-   */
-  @Test
-  public void statusWithStalePidFileReturnsNotResponding() {
-    givenPidFile(fakePid);
-
-    LocatorState locatorState = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build()
-        .status();
-
-    assertThat(locatorState.getStatus()).isEqualTo(NOT_RESPONDING);
-  }
-
   @Test
   public void stopWithPidReturnsStopped() {
     givenRunningLocator(withPort(0));

diff --git a/...integrationTest/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java b/...integrationTest/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java
@@ -14,7 +14,6 @@
  */
 package org.apache.geode.distributed;
 
-import static org.apache.geode.distributed.AbstractLauncher.Status.NOT_RESPONDING;
 import static org.apache.geode.distributed.AbstractLauncher.Status.ONLINE;
 import static org.apache.geode.distributed.AbstractLauncher.Status.STOPPED;
 import static org.apache.geode.distributed.ConfigurationProperties.DISABLE_AUTO_RECONNECT;
@@ -308,6 +307,13 @@ public void statusWithPidReturnsOnlineWithDetails() throws UnknownHostException
   public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws UnknownHostException {
     givenRunningServer();
 
+    // Keep it as we are going to overwrite it
+    int pid = readPidFile();
+
+    // Sets a fake PID to ensure that the request is going through FileProcessController,
+    // because if not, the request would fail due to the PID not existing
+    givenPidFile(fakePid);
+
     ServerState serverState = new Builder()
         .setWorkingDirectory(getWorkingDirectoryPath())
         .build()
@@ -320,64 +326,12 @@ public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws UnknownH
     assertThat(serverState.getJvmArguments()).isEqualTo(getJvmArguments());
     assertThat(serverState.getLogFile()).isEqualTo(getLogFilePath());
     assertThat(serverState.getMemberName()).isEqualTo(getUniqueName());
-    assertThat(serverState.getPid().intValue()).isEqualTo(readPidFile());
+    assertThat(serverState.getPid().intValue()).isEqualTo(pid);
     assertThat(serverState.getStatus()).isEqualTo(ONLINE);
     assertThat(serverState.getUptime()).isGreaterThan(0);
     assertThat(serverState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath());
   }
 
-  @Test
-  public void statusWithEmptyPidFileThrowsIllegalArgumentException() {
-    givenEmptyPidFile();
-    ServerLauncher launcher = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build();
-
-    Throwable thrown = catchThrowable(() -> launcher.status());
-
-    assertThat(thrown)
-        .isInstanceOf(IllegalArgumentException.class)
-        .hasMessageContaining("Invalid pid 'null' found in");
-  }
-
-  @Test
-  public void statusWithEmptyWorkingDirectoryReturnsNotRespondingWithDetails()
-      throws UnknownHostException {
-    givenEmptyWorkingDirectory();
-
-    ServerState serverState = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build()
-        .status();
-
-    assertThat(serverState.getClasspath()).isNull();
-    assertThat(serverState.getGemFireVersion()).isEqualTo(GemFireVersion.getGemFireVersion());
-    assertThat(serverState.getHost()).isEqualTo(getLocalHost().getCanonicalHostName());
-    assertThat(serverState.getJavaVersion()).isEqualTo(System.getProperty("java.version"));
-    assertThat(serverState.getJvmArguments()).isEqualTo(getJvmArguments());
-    assertThat(serverState.getLogFile()).isNull();
-    assertThat(serverState.getMemberName()).isNull();
-    assertThat(serverState.getPid()).isNull();
-    assertThat(serverState.getStatus()).isEqualTo(NOT_RESPONDING);
-    assertThat(serverState.getUptime().intValue()).isEqualTo(0);
-    assertThat(serverState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath());
-  }
-
-  /**
-   * This test takes > 1 minute to run in {@link ServerLauncherLocalFileIntegrationTest}.
-   */
-  @Test
-  public void statusWithStalePidFileReturnsNotResponding() {
-    givenPidFile(fakePid);
-
-    ServerState serverState = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build()
-        .status();
-
-    assertThat(serverState.getStatus()).isEqualTo(NOT_RESPONDING);
-  }
-
   @Test
   public void stopWithPidReturnsStopped() {
     givenRunningServer();

diff --git a/...ntegrationTest/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java b/...ntegrationTest/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java
@@ -14,14 +14,12 @@
  */
 package org.apache.geode.distributed;
 
-import static org.apache.geode.distributed.AbstractLauncher.Status.NOT_RESPONDING;
 import static org.apache.geode.distributed.AbstractLauncher.Status.ONLINE;
 import static org.apache.geode.distributed.AbstractLauncher.Status.STOPPED;
 import static org.apache.geode.distributed.ConfigurationProperties.CACHE_XML_FILE;
 import static org.apache.geode.internal.inet.LocalHostUtil.getLocalHost;
 import static org.apache.geode.util.internal.GeodeGlossary.GEMFIRE_PREFIX;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.catchThrowable;
 
 import java.io.File;
 import java.io.IOException;
@@ -210,13 +208,20 @@ public void statusWithPidReturnsOnlineWithDetails() throws IOException {
   public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws IOException {
     givenRunningServer();
 
+    // Keep it as we are going to overwrite it
+    int pid = readPidFile();
+
+    // Sets a fake PID to ensure that the request is going through FileProcessController,
+    // because if not, the request would fail due to the PID not existing
+    givenPidFile(fakePid);
+
     ServerState serverState = new Builder()
         .setWorkingDirectory(getWorkingDirectoryPath())
         .build()
         .status();
 
     assertThat(serverState.getStatus()).isEqualTo(ONLINE);
-    assertThat(serverState.getPid().intValue()).isEqualTo(readPidFile());
+    assertThat(serverState.getPid().intValue()).isEqualTo(pid);
     assertThat(serverState.getUptime()).isGreaterThan(0);
     assertThat(serverState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath());
     assertThat(serverState.getJvmArguments()).isEqualTo(getJvmArguments());
@@ -228,54 +233,6 @@ public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws IOExcept
     assertThat(serverState.getMemberName()).isEqualTo(getUniqueName());
   }
 
-  @Test
-  public void statusWithEmptyPidFileThrowsIllegalArgumentException() {
-    givenEmptyPidFile();
-
-    ServerLauncher launcher = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build();
-
-    Throwable thrown = catchThrowable(() -> launcher.status());
-
-    assertThat(thrown)
-        .isInstanceOf(IllegalArgumentException.class)
-        .hasMessageContaining("Invalid pid 'null' found in");
-  }
-
-  @Test
-  public void statusWithEmptyWorkingDirectoryReturnsNotRespondingWithDetails() throws IOException {
-    givenEmptyWorkingDirectory();
-
-    ServerState serverState = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build()
-        .status();
-
-    assertThat(serverState.getStatus()).isEqualTo(NOT_RESPONDING);
-    assertThat(serverState.getPid()).isNull();
-    assertThat(serverState.getUptime().intValue()).isEqualTo(0);
-    assertThat(serverState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath());
-    assertThat(serverState.getClasspath()).isNull();
-    assertThat(serverState.getGemFireVersion()).isEqualTo(GemFireVersion.getGemFireVersion());
-    assertThat(serverState.getJavaVersion()).isEqualTo(System.getProperty("java.version"));
-    assertThat(serverState.getLogFile()).isNull();
-    assertThat(serverState.getHost()).isEqualTo(getLocalHost().getCanonicalHostName());
-    assertThat(serverState.getMemberName()).isNull();
-  }
-
-  @Test
-  public void statusWithStalePidFileReturnsNotResponding() {
-    givenPidFile(fakePid);
-
-    ServerState serverState = new Builder()
-        .setWorkingDirectory(getWorkingDirectoryPath())
-        .build()
-        .status();
-
-    assertThat(serverState.getStatus()).isEqualTo(NOT_RESPONDING);
-  }
-
   @Test
   public void stopWithPidReturnsStopped() {
     givenRunningServer();

diff --git a/...nTest/java/org/apache/geode/internal/process/ProcessControllerFactoryIntegrationTest.java b/...nTest/java/org/apache/geode/internal/process/ProcessControllerFactoryIntegrationTest.java
@@ -19,6 +19,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 import java.io.File;
 import java.io.FileNotFoundException;
@@ -83,6 +84,19 @@ public void createProcessController_returnsMBeanProcessController() throws Excep
     assertThat(controller).isInstanceOf(MBeanOrFileProcessController.class);
   }
 
+  @Test
+  public void createProcessController_returnsFileProcessController() throws Exception {
+    // arrange
+    when(parameters.getDirectory()).thenReturn(new File("./"));
+
+    // act
+    ProcessController controller =
+        factory.createProcessController(parameters);
+
+    // assert
+    assertThat(controller).isInstanceOf(FileProcessController.class);
+  }
+
   @Test
   public void createProcessController_withoutAttachAPI_returnsFileProcessController()
       throws Exception {