Migrate business logic into NodeHealthAPI, and fix enum issue

Author: epugh

solr/core/src/java/org/apache/solr/handler/admin/HealthCheckHandler.java

@@ -23,8 +23,6 @@
 import org.apache.solr.api.Api;
 import org.apache.solr.api.JerseyResource;
 import org.apache.solr.client.solrj.request.HealthCheckRequest;
-import org.apache.solr.cloud.CloudDescriptor;
-import org.apache.solr.common.cloud.ClusterState;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.core.CoreContainer;
 import org.apache.solr.handler.RequestHandlerBase;
@@ -92,17 +90,6 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
         new NodeHealthAPI(coreContainer).checkNodeHealth(requireHealthyCores, maxGenerationLag));
   }
 
-  /**
-   * Find replicas DOWN or RECOVERING, or replicas in clusterstate that do not exist on local node.
-   *
-   * @deprecated Use {@link NodeHealthAPI#findUnhealthyCores(Collection, ClusterState)} instead.
-   */
-  @Deprecated
-  public static int findUnhealthyCores(
-      Collection<CloudDescriptor> cores, ClusterState clusterState) {
-    return NodeHealthAPI.findUnhealthyCores(cores, clusterState);
-  }
-
   @Override
   public String getDescription() {
     return "Health check handler for SolrCloud node";

solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealthAPI.java

@@ -43,6 +43,7 @@
 import org.apache.solr.common.cloud.ZkStateReader;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.CoreDescriptor;
 import org.apache.solr.core.SolrCore;
 import org.apache.solr.handler.IndexFetcher;
 import org.apache.solr.handler.ReplicationHandler;
@@ -108,24 +109,15 @@ public NodeHealthResponse checkNodeHealth(Boolean requireHealthyCores, Integer m
   }
 
   private void healthCheckCloudMode(NodeHealthResponse response, Boolean requireHealthyCores) {
-    ZkStateReader zkStateReader = coreContainer.getZkController().getZkStateReader();
-    ClusterState clusterState = zkStateReader.getClusterState();
-
-    if (zkStateReader.getZkClient().isClosed() || !zkStateReader.getZkClient().isConnected()) {
-      throw new SolrException(SERVICE_UNAVAILABLE, "Host Unavailable: Not connected to zk");
-    }
-
-    if (!clusterState.getLiveNodes().contains(coreContainer.getZkController().getNodeName())) {
-      throw new SolrException(SERVICE_UNAVAILABLE, "Host Unavailable: Not in live nodes as per zk");
-    }
+    ClusterState clusterState = getClusterState();
 
     if (Boolean.TRUE.equals(requireHealthyCores)) {
       if (!coreContainer.isStatusLoadComplete()) {
         throw new SolrException(SERVICE_UNAVAILABLE, "Host Unavailable: Core Loading not complete");
       }
       Collection<CloudDescriptor> coreDescriptors =
           coreContainer.getCoreDescriptors().stream()
-              .map(cd -> cd.getCloudDescriptor())
+              .map(CoreDescriptor::getCloudDescriptor)
               .collect(Collectors.toList());
       int unhealthyCores = findUnhealthyCores(coreDescriptors, clusterState);
       if (unhealthyCores > 0) {
@@ -143,6 +135,20 @@ private void healthCheckCloudMode(NodeHealthResponse response, Boolean requireHe
     response.status = OK;
   }
 
+  private ClusterState getClusterState() {
+    ZkStateReader zkStateReader = coreContainer.getZkController().getZkStateReader();
+    ClusterState clusterState = zkStateReader.getClusterState();
+
+    if (zkStateReader.getZkClient().isClosed() || !zkStateReader.getZkClient().isConnected()) {
+      throw new SolrException(SERVICE_UNAVAILABLE, "Host Unavailable: Not connected to zk");
+    }
+
+    if (!clusterState.getLiveNodes().contains(coreContainer.getZkController().getNodeName())) {
+      throw new SolrException(SERVICE_UNAVAILABLE, "Host Unavailable: Not in live nodes as per zk");
+    }
+    return clusterState;
+  }
+
   private void healthCheckLegacyMode(NodeHealthResponse response, Integer maxGenerationLag) {
     List<String> laggingCoresInfo = new ArrayList<>();
     boolean allCoresAreInSync = true;

solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java

@@ -23,18 +23,15 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Properties;
-import org.apache.solr.client.api.model.NodeHealthResponse;
 import org.apache.solr.client.solrj.RemoteSolrException;
 import org.apache.solr.client.solrj.SolrClient;
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.client.solrj.request.GenericSolrRequest;
 import org.apache.solr.client.solrj.request.HealthCheckRequest;
-import org.apache.solr.client.solrj.request.V2Request;
 import org.apache.solr.client.solrj.response.CollectionAdminResponse;
 import org.apache.solr.client.solrj.response.HealthCheckResponse;
-import org.apache.solr.client.solrj.response.V2Response;
 import org.apache.solr.cloud.CloudDescriptor;
 import org.apache.solr.cloud.ClusterStateMockUtil;
 import org.apache.solr.cloud.SolrCloudTestCase;
@@ -106,11 +103,7 @@ public void testHealthCheckHandler() throws Exception {
 
       // negative check of our (new) "broken" node that we deliberately put into an unhealthy state
       RemoteSolrException e =
-          expectThrows(
-              RemoteSolrException.class,
-              () -> {
-                runHealthcheckWithClient(solrClient);
-              });
+          expectThrows(RemoteSolrException.class, () -> runHealthcheckWithClient(solrClient));
       assertTrue(e.getMessage(), e.getMessage().contains("Host Unavailable"));
       assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, e.code());
     } finally {
@@ -137,48 +130,6 @@ public void testHealthCheckHandlerSolrJ() throws IOException, SolrServerExceptio
     }
   }
 
-  @Test
-  public void testHealthCheckV2Api() throws Exception {
-    V2Response res = new V2Request.Builder("/node/health").build().process(cluster.getSolrClient());
-    assertEquals(0, res.getStatus());
-    var b = res.getResponse().get(CommonParams.STATUS);
-    var c = NodeHealthResponse.NodeStatus.OK;
-    // assertEquals(c, b);
-    assertTrue(b.toString().contains(("OK")));
-
-    // add a new node for the purpose of negative testing
-    JettySolrRunner newJetty = cluster.startJettySolrRunner();
-    try (SolrClient solrClient = getHttpSolrClient(newJetty.getBaseUrl().toString())) {
-
-      // positive check that our (new) "healthy" node works with direct http client
-      var d =
-          new V2Request.Builder("/node/health")
-              .build()
-              .process(solrClient)
-              .getResponse()
-              .get(CommonParams.STATUS);
-      // assertEquals(
-      //   NodeHealthResponse.NodeStatus.OK,
-      // );
-      assertTrue(d.toString().contains(("OK")));
-
-      // now "break" our (new) node
-      newJetty.getCoreContainer().getZkController().getZkClient().close();
-
-      // negative check of our (new) "broken" node that we deliberately put into an unhealthy state
-      RemoteSolrException e =
-          expectThrows(
-              RemoteSolrException.class,
-              () -> {
-                new V2Request.Builder("/node/health").build().process(solrClient);
-              });
-      assertTrue(e.getMessage(), e.getMessage().contains("Host Unavailable"));
-      assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, e.code());
-    } finally {
-      newJetty.stop();
-    }
-  }
-
   @Test
   public void testFindUnhealthyCores() {
     // Simulate two nodes, with two collections: