gerlowskija commented on code in PR #4171: URL: https://github.com/apache/solr/pull/4171#discussion_r2878373838
########## changelog/unreleased/migrate-node-health-api.yml: ########## @@ -0,0 +1,7 @@ +title: Migrate NodeHealthAPI to JAX-RS. NodeHealthAPI now has OpenAPI and SolrJ support. +type: changed +authors: + - name: Eric Pugh +links: +- name: PR#4171 + url: https://github.com/apache/solr/pull/4171 Review Comment: [0] Link to JIRA ########## solr/api/src/java/org/apache/solr/client/api/model/NodeHealthResponse.java: ########## @@ -14,7 +14,17 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +package org.apache.solr.client.api.model; -package org.apache.solr.ui.errors +import com.fasterxml.jackson.annotation.JsonProperty; -class InvalidResponseException(message: String? = null) : Throwable(message) +/** Response body for the '/api/node/health' endpoint. */ +public class NodeHealthResponse extends SolrJerseyResponse { + + @JsonProperty public String status; Review Comment: [Q] Does status have certain expected values? If so, those can be documented using a `@Schema` annotation, or the whole field can be made an 'enum' which is probably even better. ########## solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthAPITest.java: ########## @@ -0,0 +1,150 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.handler.admin.api; + +import static org.apache.solr.common.params.CommonParams.FAILURE; +import static org.apache.solr.common.params.CommonParams.OK; + +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.SolrException; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.core.NodeConfig; +import org.apache.solr.embedded.JettySolrRunner; +import org.apache.solr.update.UpdateShardHandlerConfig; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for {@link NodeHealthAPI} that use real Solr instances instead of Mockito + * mocks. + * + * <p>Cloud-mode tests use a real {@link org.apache.solr.cloud.MiniSolrCloudCluster} and get a + * {@link CoreContainer} directly from a {@link JettySolrRunner}. Legacy (standalone) mode tests Review Comment: [Q] Do we have other tests that do standalone testing within a SolrCloudTestCase? It feels weird conceptually. And in practical terms SolrCloudTestCase does some work that makes it much slower on a per-test basis than our other base classes. Doing standalone testing in a SolrCloudTestCase is going to end up paying that runtime cost for no reason. ########## solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthAPITest.java: ########## @@ -0,0 +1,150 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.handler.admin.api; + +import static org.apache.solr.common.params.CommonParams.FAILURE; +import static org.apache.solr.common.params.CommonParams.OK; + +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.SolrException; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.core.NodeConfig; +import org.apache.solr.embedded.JettySolrRunner; +import org.apache.solr.update.UpdateShardHandlerConfig; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for {@link NodeHealthAPI} that use real Solr instances instead of Mockito Review Comment: [Q] If these are intended to be integration tests, why aren't they using the HTTP APIs the way a user would? This would be an excellent place IMO to make use of the generated SolrRequest class. Doing so would kill two birds with one stone: making the tests more of a true integration test, and getting some validation on the SolrJ code as well. ########## changelog/unreleased/migrate-node-health-api.yml: ########## @@ -0,0 +1,7 @@ +title: Migrate NodeHealthAPI to JAX-RS. NodeHealthAPI now has OpenAPI and SolrJ support. Review Comment: [0] I'd try to focus this changelog more on what matters to the user. Specifically - what is the new SolrJ class for and where can a user find it? i.e. > SolrJ now offers a SolrRequest class allowing users to perform single-node healthchecks: `NodeApi.CheckNodeHealth` Something like that... ########## solr/api/src/java/org/apache/solr/client/api/endpoint/NodeHealthApi.java: ########## @@ -14,25 +14,22 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +package org.apache.solr.client.api.endpoint; -package org.apache.solr.ui.domain +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.QueryParam; +import org.apache.solr.client.api.model.NodeHealthResponse; -import io.ktor.http.Url -import kotlinx.serialization.SerialName -import kotlinx.serialization.Serializable +/** V2 API definition for checking the health of a Solr node. */ +@Path("/node/health") +public interface NodeHealthApi { -@Serializable -data class OAuthData( - val clientId: String, - val authorizationFlow: AuthorizationFlow, - val scope: String, - val redirectUris: List<Url>, - val authorizationEndpoint: Url, - val tokenEndpoint: Url, -) - -@Serializable -enum class AuthorizationFlow { - Unknown, - CodePKCE, + @GET + @Operation( + summary = "Determine the health of a Solr node.", + tags = {"node"}) + NodeHealthResponse checkNodeHealth( Review Comment: [0] By default, the name of the generated SolrRequest class is: `<tagVal>Api.<MethodName>`. We can override that if desired by specifying an `operationId` parameter in the `@Operation` annotation, in which case the pattern becomes: `<tagVal>Api.<OperationId>` Do you like `NodeApi.CheckNodeHealth` as a classname, or would `NodeApi.Healthcheck` or something similar be better? ########## solr/core/src/java/org/apache/solr/handler/admin/HealthCheckHandler.java: ########## @@ -100,138 +103,125 @@ public CoreContainer getCoreContainer() { @Override public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception { rsp.setHttpCaching(false); + final Boolean requireHealthyCores = req.getParams().getBool(PARAM_REQUIRE_HEALTHY_CORES); + final Integer maxGenerationLag = + req.getParams().getInt(HealthCheckRequest.PARAM_MAX_GENERATION_LAG); + V2ApiUtils.squashIntoSolrResponseWithoutHeader( + rsp, checkNodeHealth(requireHealthyCores, maxGenerationLag)); + } - // Core container should not be null and active (redundant check) + /** + * Performs the node health check and returns the result as a {@link NodeHealthResponse}. + * + * <p>This method is the shared implementation used by both the v1 {@link #handleRequestBody} path + * and the v2 JAX-RS {@link NodeHealthAPI}. + */ + public NodeHealthResponse checkNodeHealth(Boolean requireHealthyCores, Integer maxGenerationLag) { Review Comment: [0] As mentioned elsewhere, this logic should probably live in `NodeHealthAPI` instead of here. ########## changelog/unreleased/migrate-node-health-api.yml: ########## @@ -0,0 +1,7 @@ +title: Migrate NodeHealthAPI to JAX-RS. NodeHealthAPI now has OpenAPI and SolrJ support. +type: changed Review Comment: [0] From a user's perspective - this is "added". There's a new SolrJ capability they didn't have before! ########## solr/core/src/java/org/apache/solr/handler/admin/HealthCheckHandler.java: ########## @@ -100,138 +103,125 @@ public CoreContainer getCoreContainer() { @Override public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception { rsp.setHttpCaching(false); + final Boolean requireHealthyCores = req.getParams().getBool(PARAM_REQUIRE_HEALTHY_CORES); + final Integer maxGenerationLag = + req.getParams().getInt(HealthCheckRequest.PARAM_MAX_GENERATION_LAG); + V2ApiUtils.squashIntoSolrResponseWithoutHeader( + rsp, checkNodeHealth(requireHealthyCores, maxGenerationLag)); + } - // Core container should not be null and active (redundant check) + /** + * Performs the node health check and returns the result as a {@link NodeHealthResponse}. + * + * <p>This method is the shared implementation used by both the v1 {@link #handleRequestBody} path + * and the v2 JAX-RS {@link NodeHealthAPI}. + */ + public NodeHealthResponse checkNodeHealth(Boolean requireHealthyCores, Integer maxGenerationLag) { if (coreContainer == null || coreContainer.isShutDown()) { - rsp.setException( - new SolrException( - SolrException.ErrorCode.SERVER_ERROR, - "CoreContainer is either not initialized or shutting down")); - return; + throw new SolrException( + SERVER_ERROR, "CoreContainer is either not initialized or shutting down"); } + + final NodeHealthResponse response = new NodeHealthResponse(); + if (!coreContainer.isZooKeeperAware()) { if (log.isDebugEnabled()) { log.debug("Invoked HealthCheckHandler in legacy mode."); } - healthCheckLegacyMode(req, rsp); + healthCheckLegacyMode(response, maxGenerationLag); } else { if (log.isDebugEnabled()) { log.debug( "Invoked HealthCheckHandler in cloud mode on [{}]", - this.coreContainer.getZkController().getNodeName()); + coreContainer.getZkController().getNodeName()); } - healthCheckCloudMode(req, rsp); + healthCheckCloudMode(response, requireHealthyCores); } + + return response; } - private void healthCheckCloudMode(SolrQueryRequest req, SolrQueryResponse rsp) { + private void healthCheckCloudMode(NodeHealthResponse response, Boolean requireHealthyCores) { ZkStateReader zkStateReader = coreContainer.getZkController().getZkStateReader(); ClusterState clusterState = zkStateReader.getClusterState(); - // Check for isConnected and isClosed + if (zkStateReader.getZkClient().isClosed() || !zkStateReader.getZkClient().isConnected()) { - rsp.add(STATUS, FAILURE); - rsp.setException( - new SolrException( - SolrException.ErrorCode.SERVICE_UNAVAILABLE, - "Host Unavailable: Not connected to zk")); - return; + throw new SolrException(SERVICE_UNAVAILABLE, "Host Unavailable: Not connected to zk"); } - // Fail if not in live_nodes if (!clusterState.getLiveNodes().contains(coreContainer.getZkController().getNodeName())) { - rsp.add(STATUS, FAILURE); - rsp.setException( - new SolrException( - SolrException.ErrorCode.SERVICE_UNAVAILABLE, - "Host Unavailable: Not in live nodes as per zk")); - return; + throw new SolrException(SERVICE_UNAVAILABLE, "Host Unavailable: Not in live nodes as per zk"); Review Comment: [Q] Does throwing an exception here actually translate to returning a `{status: failure}` field to the user as the old code did? At a glance this looks like a breaking change that'll impact both v1 and v2. And that goes for other error-scenarios in this file... ########## solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealthAPI.java: ########## @@ -17,32 +17,44 @@ package org.apache.solr.handler.admin.api; -import static org.apache.solr.client.solrj.SolrRequest.METHOD.GET; import static org.apache.solr.security.PermissionNameProvider.Name.HEALTH_PERM; -import org.apache.solr.api.EndPoint; +import jakarta.inject.Inject; +import org.apache.solr.api.JerseyResource; +import org.apache.solr.client.api.endpoint.NodeHealthApi; +import org.apache.solr.client.api.model.NodeHealthResponse; +import org.apache.solr.core.CoreContainer; import org.apache.solr.handler.admin.HealthCheckHandler; -import org.apache.solr.request.SolrQueryRequest; -import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.jersey.PermissionName; /** * V2 API for checking the health of the receiving node. * * <p>This API (GET /v2/node/health) is analogous to the v1 /admin/info/health. + * + * <p>This is a thin JAX-RS wrapper; the health-check logic lives in {@link HealthCheckHandler}. */ -public class NodeHealthAPI { +public class NodeHealthAPI extends JerseyResource implements NodeHealthApi { Review Comment: [-0] The naming convention we've used up to this point is that the `solr/api` interface gets a name ending in `Api`, and the implementing Java class **doesn't**. It's not a great convention and it has some issues, but that's what we've been following up to this point. So if it wants to follow that convention this class should prob be renamed to just `NodeHealth`. ########## solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealthAPI.java: ########## @@ -17,32 +17,44 @@ package org.apache.solr.handler.admin.api; -import static org.apache.solr.client.solrj.SolrRequest.METHOD.GET; import static org.apache.solr.security.PermissionNameProvider.Name.HEALTH_PERM; -import org.apache.solr.api.EndPoint; +import jakarta.inject.Inject; +import org.apache.solr.api.JerseyResource; +import org.apache.solr.client.api.endpoint.NodeHealthApi; +import org.apache.solr.client.api.model.NodeHealthResponse; +import org.apache.solr.core.CoreContainer; import org.apache.solr.handler.admin.HealthCheckHandler; -import org.apache.solr.request.SolrQueryRequest; -import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.jersey.PermissionName; /** * V2 API for checking the health of the receiving node. * * <p>This API (GET /v2/node/health) is analogous to the v1 /admin/info/health. + * + * <p>This is a thin JAX-RS wrapper; the health-check logic lives in {@link HealthCheckHandler}. */ -public class NodeHealthAPI { +public class NodeHealthAPI extends JerseyResource implements NodeHealthApi { + private final HealthCheckHandler handler; - public NodeHealthAPI(HealthCheckHandler handler) { - this.handler = handler; + @Inject + public NodeHealthAPI(CoreContainer coreContainer) { + this.handler = new HealthCheckHandler(coreContainer); + } + + @Override + @PermissionName(HEALTH_PERM) + public NodeHealthResponse checkNodeHealth(Boolean requireHealthyCores) { + return handler.checkNodeHealth(requireHealthyCores, null); } - // TODO Update permission here once SOLR-11623 lands. - @EndPoint( - path = {"/node/health"}, - method = GET, - permission = HEALTH_PERM) - public void getSystemInformation(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception { - handler.handleRequestBody(req, rsp); + /** + * Convenience overload used by tests and the v1 handler to pass both health-check parameters. + * + * @see HealthCheckHandler#checkNodeHealth(Boolean, Integer) + */ + public NodeHealthResponse checkNodeHealth(Boolean requireHealthyCores, Integer maxGenerationLag) { Review Comment: [-0] So, if I understand this right the v1 RequestHandler exposes this parameter, but we're not exposing it in v2. Why not? Has there been discussion on that already that I'm missing? [0] Slightly pedantic, but if a method is test-only it probably doesn't need Javadoc comments ########## solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealthAPI.java: ########## @@ -17,32 +17,44 @@ package org.apache.solr.handler.admin.api; -import static org.apache.solr.client.solrj.SolrRequest.METHOD.GET; import static org.apache.solr.security.PermissionNameProvider.Name.HEALTH_PERM; -import org.apache.solr.api.EndPoint; +import jakarta.inject.Inject; +import org.apache.solr.api.JerseyResource; +import org.apache.solr.client.api.endpoint.NodeHealthApi; +import org.apache.solr.client.api.model.NodeHealthResponse; +import org.apache.solr.core.CoreContainer; import org.apache.solr.handler.admin.HealthCheckHandler; -import org.apache.solr.request.SolrQueryRequest; -import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.jersey.PermissionName; /** * V2 API for checking the health of the receiving node. * * <p>This API (GET /v2/node/health) is analogous to the v1 /admin/info/health. + * + * <p>This is a thin JAX-RS wrapper; the health-check logic lives in {@link HealthCheckHandler}. */ -public class NodeHealthAPI { +public class NodeHealthAPI extends JerseyResource implements NodeHealthApi { + private final HealthCheckHandler handler; - public NodeHealthAPI(HealthCheckHandler handler) { - this.handler = handler; + @Inject + public NodeHealthAPI(CoreContainer coreContainer) { + this.handler = new HealthCheckHandler(coreContainer); + } + + @Override + @PermissionName(HEALTH_PERM) + public NodeHealthResponse checkNodeHealth(Boolean requireHealthyCores) { + return handler.checkNodeHealth(requireHealthyCores, null); Review Comment: [-1] The goal with v2 is to structure things so that the v1 RequestHandler consumes the v2 API for the logic it needs. v1 should "just" be a layer on top of v2 that can be deprecated and deleted trivially down the road. Having the v2 API rely on the RequestHandler here is kindof backwards of what we want. Can we rework things to drop that HealthCheckHandler dependency? If there's logic in that handler that we need, let's move it here (or even better - into a separate utility class) ########## solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthAPITest.java: ########## @@ -0,0 +1,150 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.handler.admin.api; + +import static org.apache.solr.common.params.CommonParams.FAILURE; +import static org.apache.solr.common.params.CommonParams.OK; + +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.SolrException; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.core.NodeConfig; +import org.apache.solr.embedded.JettySolrRunner; +import org.apache.solr.update.UpdateShardHandlerConfig; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for {@link NodeHealthAPI} that use real Solr instances instead of Mockito + * mocks. + * + * <p>Cloud-mode tests use a real {@link org.apache.solr.cloud.MiniSolrCloudCluster} and get a + * {@link CoreContainer} directly from a {@link JettySolrRunner}. Legacy (standalone) mode tests + * create an embedded {@link CoreContainer} via {@link NodeConfig} with no ZooKeeper. + */ +public class NodeHealthAPITest extends SolrCloudTestCase { + + @BeforeClass + public static void setupCluster() throws Exception { + configureCluster(1).addConfig("conf", configset("cloud-minimal")).configure(); + } + + // ---- Cloud (ZooKeeper) mode tests ---- + + @Test + public void testCloudMode_HealthyNodeReturnsOkStatus() { + CoreContainer coreContainer = cluster.getJettySolrRunner(0).getCoreContainer(); + + final var response = new NodeHealthAPI(coreContainer).checkNodeHealth(null); + + assertNotNull(response); + assertEquals(OK, response.status); + assertNull("Expected no error on a healthy node", response.error); + } + + @Test + public void testCloudMode_RequireHealthyCoresReturnOkWhenAllCoresHealthy() { + CoreContainer coreContainer = cluster.getJettySolrRunner(0).getCoreContainer(); + + // requireHealthyCores=true should succeed on a node with no unhealthy cores Review Comment: [-1] You haven't actually created any cores!! Can you create a collection or something that'll cause this test to actually exercise the per-core logic currently in HealthcheckHandler? ########## solr/solr-ref-guide/modules/configuration-guide/pages/implicit-requesthandlers.adoc: ########## @@ -47,7 +47,9 @@ Health:: Report the health of the node (_available only in SolrCloud mode_) |API Endpoints |Class & Javadocs |Paramset |v1: `solr/admin/info/health` -v2: `api/node/health` |{solr-javadocs}/core/org/apache/solr/handler/admin/HealthCheckHandler.html[HealthCheckHandler] | +v2: `api/node/health` |v1: {solr-javadocs}/core/org/apache/solr/handler/admin/HealthCheckHandler.html[HealthCheckHandler] + +v2: {solr-javadocs}/core/org/apache/solr/handler/admin/api/NodeHealthAPI.html[NodeHealthAPI] | Review Comment: [Q] Is it the implementation Javadocs we want to point people to here, or would the `solr/api` interface docs be more helpful? Or more broadly - is there much value even in pointing to either Javadoc on the v2 side? HealthcheckHandler has a nice good blurb, but neither NodeHealthAPI nor NodeHealthApi have much of anything that's worth pointing a user at IMO... ########## solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthAPITest.java: ########## @@ -0,0 +1,150 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.handler.admin.api; + +import static org.apache.solr.common.params.CommonParams.FAILURE; +import static org.apache.solr.common.params.CommonParams.OK; + +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.SolrException; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.core.NodeConfig; +import org.apache.solr.embedded.JettySolrRunner; +import org.apache.solr.update.UpdateShardHandlerConfig; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for {@link NodeHealthAPI} that use real Solr instances instead of Mockito + * mocks. + * + * <p>Cloud-mode tests use a real {@link org.apache.solr.cloud.MiniSolrCloudCluster} and get a + * {@link CoreContainer} directly from a {@link JettySolrRunner}. Legacy (standalone) mode tests + * create an embedded {@link CoreContainer} via {@link NodeConfig} with no ZooKeeper. + */ +public class NodeHealthAPITest extends SolrCloudTestCase { + + @BeforeClass + public static void setupCluster() throws Exception { + configureCluster(1).addConfig("conf", configset("cloud-minimal")).configure(); + } + + // ---- Cloud (ZooKeeper) mode tests ---- + + @Test + public void testCloudMode_HealthyNodeReturnsOkStatus() { + CoreContainer coreContainer = cluster.getJettySolrRunner(0).getCoreContainer(); + + final var response = new NodeHealthAPI(coreContainer).checkNodeHealth(null); + + assertNotNull(response); + assertEquals(OK, response.status); + assertNull("Expected no error on a healthy node", response.error); + } + + @Test + public void testCloudMode_RequireHealthyCoresReturnOkWhenAllCoresHealthy() { + CoreContainer coreContainer = cluster.getJettySolrRunner(0).getCoreContainer(); + + // requireHealthyCores=true should succeed on a node with no unhealthy cores + final var response = new NodeHealthAPI(coreContainer).checkNodeHealth(true); + + assertNotNull(response); + assertEquals(OK, response.status); + assertEquals("All cores are healthy", response.message); + } + + @Test + public void testCloudMode_UnhealthyWhenZkClientClosed() throws Exception { + // Use a fresh node so closing its ZK client does not break the primary cluster node + JettySolrRunner newJetty = cluster.startJettySolrRunner(); + try { + CoreContainer coreContainer = newJetty.getCoreContainer(); + + // Sanity check: the new node should start out healthy + assertEquals(OK, new NodeHealthAPI(coreContainer).checkNodeHealth(null).status); + + // Break the ZK connection to put the node into an unhealthy state + coreContainer.getZkController().getZkClient().close(); + + SolrException e = + assertThrows( + SolrException.class, () -> new NodeHealthAPI(coreContainer).checkNodeHealth(null)); + assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, e.code()); + assertTrue( + "Expected 'Host Unavailable' in exception message", + e.getMessage().contains("Host Unavailable")); + } finally { + newJetty.stop(); + } + } + + // ---- Legacy (standalone, non-ZooKeeper) mode tests ---- Review Comment: [Q] Are we calling this "Legacy" now? 🤔 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
