Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-4150237081 > HealthCheckHandlerTest Thanks David! I took a quick looksee to see if it was like a deprecated class we could use, or just add a supress ssl, but sounds like you have a better plan? Awesome. -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
dsmiley commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-4148849209 This has a flaky test: > ./gradlew :solr:core:test --tests "org.apache.solr.handler.admin.HealthCheckHandlerTest.testV1FailureResponseIncludesStatusField" -Ptests.seed=58F0C58B3D8F9C1D Due to randomized use of SSL, we can't use the JDK HttpClient unless we configure the client for the SSL. I'm going to throw up a PR soon to fix this and make it a bit easier to be compliant. -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh merged PR #4171: URL: https://github.com/apache/solr/pull/4171 -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2965256328
##
solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealth.java:
##
@@ -0,0 +1,287 @@
+/*
+ * 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.client.api.model.NodeHealthResponse.NodeStatus.FAILURE;
+import static
org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.OK;
+import static org.apache.solr.common.SolrException.ErrorCode.SERVER_ERROR;
+import static
org.apache.solr.common.SolrException.ErrorCode.SERVICE_UNAVAILABLE;
+import static org.apache.solr.handler.admin.api.ReplicationAPIBase.GENERATION;
+import static org.apache.solr.security.PermissionNameProvider.Name.HEALTH_PERM;
+
+import jakarta.inject.Inject;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.Locale;
+import java.util.stream.Collectors;
+import org.apache.lucene.index.IndexCommit;
+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.cloud.CloudDescriptor;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.Replica.State;
+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;
+import org.apache.solr.jersey.PermissionName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * V2 API for checking the health of the receiving node.
+ *
+ * This API (GET /v2/node/health) is analogous to the v1 /admin/info/health.
+ *
+ * The v1 {@link org.apache.solr.handler.admin.HealthCheckHandler}
delegates to this class.
+ */
+public class NodeHealth extends JerseyResource implements NodeHealthApi {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private static final List UNHEALTHY_STATES =
Arrays.asList(State.DOWN, State.RECOVERING);
+
+ private final CoreContainer coreContainer;
+
+ @Inject
+ public NodeHealth(CoreContainer coreContainer) {
+this.coreContainer = coreContainer;
+ }
+
+ @Override
+ @PermissionName(HEALTH_PERM)
+ public NodeHealthResponse healthcheck(Boolean requireHealthyCores) {
+return checkNodeHealth(requireHealthyCores, null);
+ }
+
+ /**
+ * Performs the node health check and returns the result as a {@link
NodeHealthResponse}.
+ *
+ * This overload is used by the v1 {@link
+ * org.apache.solr.handler.admin.HealthCheckHandler#handleRequestBody} path,
which can supply the
+ * legacy {@code maxGenerationLag} parameter that is not exposed via the v2
endpoint.
+ */
+ public NodeHealthResponse checkNodeHealth(Boolean requireHealthyCores,
Integer maxGenerationLag) {
+if (coreContainer == null || coreContainer.isShutDown()) {
+ throw new SolrException(
+ SERVER_ERROR, "CoreContainer is either not initialized or shutting
down");
+}
+
+final NodeHealthResponse response =
instantiateJerseyResponse(NodeHealthResponse.class);
+
+if (!coreContainer.isZooKeeperAware()) {
+ if (log.isDebugEnabled()) {
+log.debug("Invoked HealthCheckHandler in legacy mode.");
+ }
+ healthCheckLegacyMode(response, maxGenerationLag);
+} else {
+ if (log.isDebugEnabled()) {
+log.debug(
+"Invoked HealthCheckHandler in cloud mode on [{}]",
+coreContainer.getZkController().getNodeName());
+ }
+ healthCheckCloudMode(response, requireHealthyCores);
+}
+
+return response;
+ }
+
+ private void healthCheckCloudMode(NodeHealthResponse response, Boolean
requireHealthyCores) {
+ClusterState clusterState = getClusterState();
+
+if (Boolean.TRUE.equals(requireHealthyCores)) {
+ if (!c
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
gerlowskija commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2960685491
##
solr/api/src/java/org/apache/solr/client/api/endpoint/NodeHealthApi.java:
##
@@ -30,5 +31,14 @@ public interface NodeHealthApi {
@Operation(
summary = "Determine the health of a Solr node.",
tags = {"node"})
- NodeHealthResponse healthcheck(@QueryParam("requireHealthyCores") Boolean
requireHealthyCores);
+ NodeHealthResponse healthcheck(
+ @QueryParam("requireHealthyCores") Boolean requireHealthyCores,
+ @Parameter(
+ description =
+ "Maximum number of index generations a follower replica may
lag behind its"
+ + " leader before the health check reports FAILURE. Only
relevant when"
+ + " running in legacy (non-SolrCloud) mode with
leader/follower"
Review Comment:
[0] I've seen you remove "legacy" a few other places based on an earlier
comment, but I think this one got missed.
I don't mind that language personally, but I don't think the community as a
whole agrees on that yet and there's probably some broader consensus-building
needed before we start calling leader/follower "legacy"
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
gerlowskija commented on code in PR #4171: URL: https://github.com/apache/solr/pull/4171#discussion_r2963560876 ## solr/solr-ref-guide/modules/deployment-guide/pages/user-managed-index-replication.adoc: ## @@ -575,6 +575,63 @@ A snapshot with the name `snapshot._name_` must exist or an error will be return `location`::: The location where the snapshot is created. +[[monitoring-follower-replication-lag]] +== Monitoring Follower Replication Lag + +In a leader-follower deployment it is important to know whether followers are keeping pace with the leader. +Solr's health-check endpoint supports a `maxGenerationLag` request parameter that lets you assert that each follower core is within a specified number of Lucene commit generations of its leader. +When the follower is lagging more than the allowed number of generations the endpoint returns HTTP 503 (Service Unavailable), making it straightforward to integrate into load-balancer health probes or monitoring systems. + +The `maxGenerationLag` parameter is an integer representing the maximum acceptable number of commit generations by which a follower is allowed to trail its leader. +A value of `0` requires the follower to be fully up to date. +If the parameter is omitted, the health check returns `OK` regardless of replication lag. + +[WARNING] + +Because a follower's generation can only increase when a replication from the leader actually completes, `maxGenerationLag=0` may return `FAILURE` immediately after a follower starts or after a period of network instability even though the follower will catch up on the next poll cycle. Review Comment: Agreed - knowing that it's coming from Javadoc content that already exists makes me much more comfortable propagating it. Missed that initially. 👍 -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
dsmiley commented on code in PR #4171: URL: https://github.com/apache/solr/pull/4171#discussion_r2962934791 ## solr/solr-ref-guide/modules/deployment-guide/pages/user-managed-index-replication.adoc: ## @@ -575,6 +575,63 @@ A snapshot with the name `snapshot._name_` must exist or an error will be return `location`::: The location where the snapshot is created. +[[monitoring-follower-replication-lag]] +== Monitoring Follower Replication Lag + +In a leader-follower deployment it is important to know whether followers are keeping pace with the leader. +Solr's health-check endpoint supports a `maxGenerationLag` request parameter that lets you assert that each follower core is within a specified number of Lucene commit generations of its leader. +When the follower is lagging more than the allowed number of generations the endpoint returns HTTP 503 (Service Unavailable), making it straightforward to integrate into load-balancer health probes or monitoring systems. + +The `maxGenerationLag` parameter is an integer representing the maximum acceptable number of commit generations by which a follower is allowed to trail its leader. +A value of `0` requires the follower to be fully up to date. +If the parameter is omitted, the health check returns `OK` regardless of replication lag. + +[WARNING] + +Because a follower's generation can only increase when a replication from the leader actually completes, `maxGenerationLag=0` may return `FAILURE` immediately after a follower starts or after a period of network instability even though the follower will catch up on the next poll cycle. Review Comment: FWIW I think it's reasonable to propagate javadocs to ref guide note if you feel it's helpful/worthwhile info there. It's not a hallucination. -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2962178321
##
solr/api/src/java/org/apache/solr/client/api/endpoint/NodeHealthApi.java:
##
@@ -30,5 +31,14 @@ public interface NodeHealthApi {
@Operation(
summary = "Determine the health of a Solr node.",
tags = {"node"})
- NodeHealthResponse healthcheck(@QueryParam("requireHealthyCores") Boolean
requireHealthyCores);
+ NodeHealthResponse healthcheck(
+ @QueryParam("requireHealthyCores") Boolean requireHealthyCores,
+ @Parameter(
+ description =
+ "Maximum number of index generations a follower replica may
lag behind its"
+ + " leader before the health check reports FAILURE. Only
relevant when"
+ + " running in legacy (non-SolrCloud) mode with
leader/follower"
Review Comment:
good catch.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171: URL: https://github.com/apache/solr/pull/4171#discussion_r2962172468 ## solr/solr-ref-guide/modules/deployment-guide/pages/user-managed-index-replication.adoc: ## @@ -575,6 +575,63 @@ A snapshot with the name `snapshot._name_` must exist or an error will be return `location`::: The location where the snapshot is created. +[[monitoring-follower-replication-lag]] +== Monitoring Follower Replication Lag + +In a leader-follower deployment it is important to know whether followers are keeping pace with the leader. +Solr's health-check endpoint supports a `maxGenerationLag` request parameter that lets you assert that each follower core is within a specified number of Lucene commit generations of its leader. +When the follower is lagging more than the allowed number of generations the endpoint returns HTTP 503 (Service Unavailable), making it straightforward to integrate into load-balancer health probes or monitoring systems. + +The `maxGenerationLag` parameter is an integer representing the maximum acceptable number of commit generations by which a follower is allowed to trail its leader. +A value of `0` requires the follower to be fully up to date. +If the parameter is omitted, the health check returns `OK` regardless of replication lag. + +[WARNING] + +Because a follower's generation can only increase when a replication from the leader actually completes, `maxGenerationLag=0` may return `FAILURE` immediately after a follower starts or after a period of network instability even though the follower will catch up on the next poll cycle. Review Comment: yeah, it was generated, and yes, you should DEFINITLY ask about these statemetns. It is documented in the javadocs in `HealthCheckHandler`. I'll be honest, I can't super vouch for this as well since I've never actually personally run leader/follower and then used this...I wonder if we are better off not having a WARNING on something that we only know from reading javadocs and haven't personally tested? I.e err on just documenting the "what we know", not "what we believe". -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171: URL: https://github.com/apache/solr/pull/4171#discussion_r2962141541 ## changelog/unreleased/SOLR-16458-migrate-node-health-api.yml: ## @@ -1,7 +1,8 @@ -title: "SolrJ now offers a SolrRequest class allowing users to perform single-node healthchecks: NodeApi.Healthcheck" +title: "SolrJ now offers a SolrRequest class allowing users to perform v2 single-node healthchecks: NodeApi.Healthcheck" type: added authors: - name: Eric Pugh + - name: Jason Gerlowski Review Comment: i added you because you went beyond review, I mean, this PR chagned a lot! Just causeyou didn't type characters doens't mean you didn't have a lot of input! I think you authored this as much as I did. -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2962142628
##
solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthTest.java:
##
@@ -85,9 +77,10 @@ public void testCloudMode_UnhealthyWhenZkClientClosed()
throws Exception {
SolrException e =
assertThrows(SolrException.class, () -> new
NodeApi.Healthcheck().process(nodeClient));
assertEquals(ErrorCode.SERVICE_UNAVAILABLE.code, e.code());
- assertTrue(
+ assertThat(
Review Comment:
i tried with and without, and yeah, this was a surpirse.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
gerlowskija commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2960614143
##
solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthTest.java:
##
@@ -28,30 +28,21 @@
import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrException.ErrorCode;
import org.apache.solr.embedded.JettySolrRunner;
-import org.apache.solr.util.SolrJettyTestRule;
import org.junit.BeforeClass;
-import org.junit.ClassRule;
import org.junit.Test;
public class NodeHealthTest extends SolrCloudTestCase {
- /**
Review Comment:
[-0] I love that this test is now SolrCloud only, but should it's name be
changed to indicate that. And maybe it should have Javadocs that link to its
standalone counterpart.
e.g.
```
/**
* Tests for the node-health API, on SolrCloud clusters
*
* @see NodeHealthStandaloneTest
*/
public class NodeHealthSolrCloudTest extends SolrCloudTestCase {
```
##
solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthTest.java:
##
@@ -85,9 +77,10 @@ public void testCloudMode_UnhealthyWhenZkClientClosed()
throws Exception {
SolrException e =
assertThrows(SolrException.class, () -> new
NodeApi.Healthcheck().process(nodeClient));
assertEquals(ErrorCode.SERVICE_UNAVAILABLE.code, e.code());
- assertTrue(
+ assertThat(
Review Comment:
[0] If you're taking this Hamcrest approach, you don't even need the first
argument: `"Expected 'host Unavailable' in exception message"`.
In fact, the default message Hamcrest will print on failure is **better**
than the override you've specified here since the default message will include
the actual message value.
##
solr/solr-ref-guide/modules/deployment-guide/pages/user-managed-index-replication.adoc:
##
@@ -575,6 +575,63 @@ A snapshot with the name `snapshot._name_` must exist or
an error will be return
`location`::: The location where the snapshot is created.
+[[monitoring-follower-replication-lag]]
+== Monitoring Follower Replication Lag
+
+In a leader-follower deployment it is important to know whether followers are
keeping pace with the leader.
+Solr's health-check endpoint supports a `maxGenerationLag` request parameter
that lets you assert that each follower core is within a specified number of
Lucene commit generations of its leader.
+When the follower is lagging more than the allowed number of generations the
endpoint returns HTTP 503 (Service Unavailable), making it straightforward to
integrate into load-balancer health probes or monitoring systems.
+
+The `maxGenerationLag` parameter is an integer representing the maximum
acceptable number of commit generations by which a follower is allowed to trail
its leader.
+A value of `0` requires the follower to be fully up to date.
+If the parameter is omitted, the health check returns `OK` regardless of
replication lag.
+
+[WARNING]
+
+Because a follower's generation can only increase when a replication from the
leader actually completes, `maxGenerationLag=0` may return `FAILURE`
immediately after a follower starts or after a period of network instability
even though the follower will catch up on the next poll cycle.
Review Comment:
[Q] Did this information come from somewhere? Or is it AI generated? (And
if the latter - is it something you're familiar with and can confirm is
correct?)
It all sounds very plausible, and if it's correct then the docs are
excellent. But I wanted to highlight that I can't validate its correctness as
a reviewer because I don't have a lot of experience deploying leader/follower
personally.
##
changelog/unreleased/SOLR-16458-migrate-node-health-api.yml:
##
@@ -1,7 +1,8 @@
-title: "SolrJ now offers a SolrRequest class allowing users to perform
single-node healthchecks: NodeApi.Healthcheck"
+title: "SolrJ now offers a SolrRequest class allowing users to perform v2
single-node healthchecks: NodeApi.Healthcheck"
type: added
authors:
- name: Eric Pugh
+ - name: Jason Gerlowski
Review Comment:
[Q] (General question; not PR specific)
Is it our convention to add reviewers here as well?
I'm all for that but it'd be great to document in `dev-docs/changelog.adoc`
if that's a convention we'd like to cement!
##
solr/api/src/java/org/apache/solr/client/api/endpoint/NodeHealthApi.java:
##
@@ -30,5 +31,14 @@ public interface NodeHealthApi {
@Operation(
summary = "Determine the health of a Solr node.",
tags = {"node"})
- NodeHealthResponse healthcheck(@QueryParam("requireHealthyCores") Boolean
requireHealthyCores);
+ NodeHealthResponse healthcheck(
+ @QueryParam("requireHealthyCores") Boolean requireHealthyCores,
+ @Parameter(
+ description =
+ "Maximum number of index generations a follower replica may
lag behind its"
+
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
gerlowskija commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-4090609183 > Do you want me to resolve conversations/comments as I fix them, or push up the fix and let you resolve them? I'm 👍 with you clicking "Resolve Conversation". I trust you're not doing it on stuff that's controversial or whatever, and it saves me from a bunch of additional reading each time I open this PR back up. (When I'm on the other side of things and doing the editing it also makes for a nice little built-in "To Do" list too...not sure if others users it that way or not) Anyway, re-reviewing 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-4089524181 And we have generation lag! Plus some actual docs about it since it was a completely NEW parameter to me. -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2959216131
##
solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealth.java:
##
@@ -0,0 +1,287 @@
+/*
+ * 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.client.api.model.NodeHealthResponse.NodeStatus.FAILURE;
+import static
org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.OK;
+import static org.apache.solr.common.SolrException.ErrorCode.SERVER_ERROR;
+import static
org.apache.solr.common.SolrException.ErrorCode.SERVICE_UNAVAILABLE;
+import static org.apache.solr.handler.admin.api.ReplicationAPIBase.GENERATION;
+import static org.apache.solr.security.PermissionNameProvider.Name.HEALTH_PERM;
+
+import jakarta.inject.Inject;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.Locale;
+import java.util.stream.Collectors;
+import org.apache.lucene.index.IndexCommit;
+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.cloud.CloudDescriptor;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.Replica.State;
+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;
+import org.apache.solr.jersey.PermissionName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * V2 API for checking the health of the receiving node.
+ *
+ * This API (GET /v2/node/health) is analogous to the v1 /admin/info/health.
+ *
+ * The v1 {@link org.apache.solr.handler.admin.HealthCheckHandler}
delegates to this class.
+ */
+public class NodeHealth extends JerseyResource implements NodeHealthApi {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private static final List UNHEALTHY_STATES =
Arrays.asList(State.DOWN, State.RECOVERING);
+
+ private final CoreContainer coreContainer;
+
+ @Inject
+ public NodeHealth(CoreContainer coreContainer) {
+this.coreContainer = coreContainer;
+ }
+
+ @Override
+ @PermissionName(HEALTH_PERM)
+ public NodeHealthResponse healthcheck(Boolean requireHealthyCores) {
+return checkNodeHealth(requireHealthyCores, null);
+ }
+
+ /**
+ * Performs the node health check and returns the result as a {@link
NodeHealthResponse}.
+ *
+ * This overload is used by the v1 {@link
+ * org.apache.solr.handler.admin.HealthCheckHandler#handleRequestBody} path,
which can supply the
+ * legacy {@code maxGenerationLag} parameter that is not exposed via the v2
endpoint.
+ */
+ public NodeHealthResponse checkNodeHealth(Boolean requireHealthyCores,
Integer maxGenerationLag) {
+if (coreContainer == null || coreContainer.isShutDown()) {
+ throw new SolrException(
+ SERVER_ERROR, "CoreContainer is either not initialized or shutting
down");
+}
+
+final NodeHealthResponse response =
instantiateJerseyResponse(NodeHealthResponse.class);
+
+if (!coreContainer.isZooKeeperAware()) {
+ if (log.isDebugEnabled()) {
+log.debug("Invoked HealthCheckHandler in legacy mode.");
+ }
+ healthCheckLegacyMode(response, maxGenerationLag);
+} else {
+ if (log.isDebugEnabled()) {
+log.debug(
+"Invoked HealthCheckHandler in cloud mode on [{}]",
+coreContainer.getZkController().getNodeName());
+ }
+ healthCheckCloudMode(response, requireHealthyCores);
+}
+
+return response;
+ }
+
+ private void healthCheckCloudMode(NodeHealthResponse response, Boolean
requireHealthyCores) {
+ClusterState clusterState = getClusterState();
+
+if (Boolean.TRUE.equals(requireHealthyCores)) {
+ if (!c
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-4089243113 @gerlowskija I have responded/fixed everything EXCEPT the generation lag.. Do you want me to resolve conversations/comments as I fix them, or push up the fix and let you resolve them? -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2959189452
##
solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealth.java:
##
@@ -0,0 +1,287 @@
+/*
+ * 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.client.api.model.NodeHealthResponse.NodeStatus.FAILURE;
+import static
org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.OK;
+import static org.apache.solr.common.SolrException.ErrorCode.SERVER_ERROR;
+import static
org.apache.solr.common.SolrException.ErrorCode.SERVICE_UNAVAILABLE;
+import static org.apache.solr.handler.admin.api.ReplicationAPIBase.GENERATION;
+import static org.apache.solr.security.PermissionNameProvider.Name.HEALTH_PERM;
+
+import jakarta.inject.Inject;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.Locale;
+import java.util.stream.Collectors;
+import org.apache.lucene.index.IndexCommit;
+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.cloud.CloudDescriptor;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.Replica.State;
+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;
+import org.apache.solr.jersey.PermissionName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * V2 API for checking the health of the receiving node.
+ *
+ * This API (GET /v2/node/health) is analogous to the v1 /admin/info/health.
+ *
+ * The v1 {@link org.apache.solr.handler.admin.HealthCheckHandler}
delegates to this class.
+ */
+public class NodeHealth extends JerseyResource implements NodeHealthApi {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private static final List UNHEALTHY_STATES =
Arrays.asList(State.DOWN, State.RECOVERING);
+
+ private final CoreContainer coreContainer;
+
+ @Inject
+ public NodeHealth(CoreContainer coreContainer) {
+this.coreContainer = coreContainer;
+ }
+
+ @Override
+ @PermissionName(HEALTH_PERM)
+ public NodeHealthResponse healthcheck(Boolean requireHealthyCores) {
+return checkNodeHealth(requireHealthyCores, null);
+ }
+
+ /**
+ * Performs the node health check and returns the result as a {@link
NodeHealthResponse}.
+ *
+ * This overload is used by the v1 {@link
+ * org.apache.solr.handler.admin.HealthCheckHandler#handleRequestBody} path,
which can supply the
+ * legacy {@code maxGenerationLag} parameter that is not exposed via the v2
endpoint.
+ */
+ public NodeHealthResponse checkNodeHealth(Boolean requireHealthyCores,
Integer maxGenerationLag) {
+if (coreContainer == null || coreContainer.isShutDown()) {
+ throw new SolrException(
+ SERVER_ERROR, "CoreContainer is either not initialized or shutting
down");
+}
+
+final NodeHealthResponse response =
instantiateJerseyResponse(NodeHealthResponse.class);
+
+if (!coreContainer.isZooKeeperAware()) {
+ if (log.isDebugEnabled()) {
+log.debug("Invoked HealthCheckHandler in legacy mode.");
+ }
+ healthCheckLegacyMode(response, maxGenerationLag);
+} else {
+ if (log.isDebugEnabled()) {
+log.debug(
+"Invoked HealthCheckHandler in cloud mode on [{}]",
+coreContainer.getZkController().getNodeName());
+ }
+ healthCheckCloudMode(response, requireHealthyCores);
+}
+
+return response;
+ }
+
+ private void healthCheckCloudMode(NodeHealthResponse response, Boolean
requireHealthyCores) {
+ClusterState clusterState = getClusterState();
+
+if (Boolean.TRUE.equals(requireHealthyCores)) {
+ if (!c
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2959180290
##
solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealth.java:
##
@@ -0,0 +1,287 @@
+/*
+ * 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.client.api.model.NodeHealthResponse.NodeStatus.FAILURE;
+import static
org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.OK;
+import static org.apache.solr.common.SolrException.ErrorCode.SERVER_ERROR;
+import static
org.apache.solr.common.SolrException.ErrorCode.SERVICE_UNAVAILABLE;
+import static org.apache.solr.handler.admin.api.ReplicationAPIBase.GENERATION;
+import static org.apache.solr.security.PermissionNameProvider.Name.HEALTH_PERM;
+
+import jakarta.inject.Inject;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.Locale;
+import java.util.stream.Collectors;
+import org.apache.lucene.index.IndexCommit;
+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.cloud.CloudDescriptor;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.Replica.State;
+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;
+import org.apache.solr.jersey.PermissionName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * V2 API for checking the health of the receiving node.
+ *
+ * This API (GET /v2/node/health) is analogous to the v1 /admin/info/health.
+ *
+ * The v1 {@link org.apache.solr.handler.admin.HealthCheckHandler}
delegates to this class.
+ */
+public class NodeHealth extends JerseyResource implements NodeHealthApi {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private static final List UNHEALTHY_STATES =
Arrays.asList(State.DOWN, State.RECOVERING);
+
+ private final CoreContainer coreContainer;
+
+ @Inject
+ public NodeHealth(CoreContainer coreContainer) {
+this.coreContainer = coreContainer;
+ }
+
+ @Override
+ @PermissionName(HEALTH_PERM)
+ public NodeHealthResponse healthcheck(Boolean requireHealthyCores) {
+return checkNodeHealth(requireHealthyCores, null);
+ }
+
+ /**
+ * Performs the node health check and returns the result as a {@link
NodeHealthResponse}.
+ *
+ * This overload is used by the v1 {@link
+ * org.apache.solr.handler.admin.HealthCheckHandler#handleRequestBody} path,
which can supply the
+ * legacy {@code maxGenerationLag} parameter that is not exposed via the v2
endpoint.
+ */
+ public NodeHealthResponse checkNodeHealth(Boolean requireHealthyCores,
Integer maxGenerationLag) {
+if (coreContainer == null || coreContainer.isShutDown()) {
+ throw new SolrException(
+ SERVER_ERROR, "CoreContainer is either not initialized or shutting
down");
+}
+
+final NodeHealthResponse response =
instantiateJerseyResponse(NodeHealthResponse.class);
+
+if (!coreContainer.isZooKeeperAware()) {
+ if (log.isDebugEnabled()) {
+log.debug("Invoked HealthCheckHandler in legacy mode.");
+ }
+ healthCheckLegacyMode(response, maxGenerationLag);
+} else {
+ if (log.isDebugEnabled()) {
+log.debug(
+"Invoked HealthCheckHandler in cloud mode on [{}]",
+coreContainer.getZkController().getNodeName());
+ }
+ healthCheckCloudMode(response, requireHealthyCores);
+}
+
+return response;
+ }
+
+ private void healthCheckCloudMode(NodeHealthResponse response, Boolean
requireHealthyCores) {
+ClusterState clusterState = getClusterState();
+
+if (Boolean.TRUE.equals(requireHealthyCores)) {
+ if (!c
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2959158008
##
solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthTest.java:
##
@@ -0,0 +1,164 @@
+/*
+ * 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.client.api.model.NodeHealthResponse.NodeStatus.FAILURE;
+import static
org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.OK;
+
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.NodeApi;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.apache.solr.util.SolrJettyTestRule;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+public class NodeHealthTest extends SolrCloudTestCase {
+
+ /**
+ * A standalone (non-ZooKeeper) Jetty instance used by the legacy-mode
tests. The
+ * {@code @ClassRule} ensures it is shut down after all tests in this class
complete.
+ */
+ @ClassRule public static SolrJettyTestRule standaloneJetty = new
SolrJettyTestRule();
Review Comment:
No pattern, so `NodeHealthStandaloneTestCase`? Hoping to get this guy
removed from Solr 11 anyway ;-). Or Solr 12.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2959126618
##
solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthTest.java:
##
@@ -0,0 +1,164 @@
+/*
+ * 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.client.api.model.NodeHealthResponse.NodeStatus.FAILURE;
+import static
org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.OK;
+
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.NodeApi;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.apache.solr.util.SolrJettyTestRule;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+public class NodeHealthTest extends SolrCloudTestCase {
+
+ /**
+ * A standalone (non-ZooKeeper) Jetty instance used by the legacy-mode
tests. The
+ * {@code @ClassRule} ensures it is shut down after all tests in this class
complete.
+ */
+ @ClassRule public static SolrJettyTestRule standaloneJetty = new
SolrJettyTestRule();
Review Comment:
I can get behind this... Especially because this extends
SolrCloudTestCase... and hoenstly, the standaloneJetty confused me as well.
I will look and see if there is a pattern...
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2959118418
##
solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthTest.java:
##
@@ -0,0 +1,164 @@
+/*
+ * 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.client.api.model.NodeHealthResponse.NodeStatus.FAILURE;
+import static
org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.OK;
+
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.NodeApi;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.apache.solr.util.SolrJettyTestRule;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+public class NodeHealthTest extends SolrCloudTestCase {
+
+ /**
+ * A standalone (non-ZooKeeper) Jetty instance used by the legacy-mode
tests. The
+ * {@code @ClassRule} ensures it is shut down after all tests in this class
complete.
+ */
+ @ClassRule public static SolrJettyTestRule standaloneJetty = new
SolrJettyTestRule();
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+configureCluster(1).addConfig("conf",
configset("cloud-minimal")).configure();
+standaloneJetty.startSolr(createTempDir());
+
+CollectionAdminRequest.createCollection(DEFAULT_TEST_COLLECTION_NAME,
"conf", 1, 1)
+.process(cluster.getSolrClient());
+ }
+
+ @Test
+ public void testCloudMode_HealthyNodeReturnsOkStatus() throws Exception {
+final var request = new NodeApi.Healthcheck();
+final var response = request.process(cluster.getSolrClient());
+
+assertNotNull(response);
+assertEquals(OK, response.status);
+assertNull("Expected no error on a healthy node", response.error);
+ }
+
+ @Test
+ public void testCloudMode_RequireHealthyCoresReturnOkWhenAllCoresHealthy()
throws Exception {
+final var request = new NodeApi.Healthcheck();
+request.setRequireHealthyCores(true);
+final var response = request.process(cluster.getSolrClient());
+
+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();
Review Comment:
I'm asking copilot to look overall at our code base... and fixing this
specfiic case.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2959049407
##
solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthTest.java:
##
@@ -0,0 +1,164 @@
+/*
+ * 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.client.api.model.NodeHealthResponse.NodeStatus.FAILURE;
+import static
org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.OK;
+
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.NodeApi;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.apache.solr.util.SolrJettyTestRule;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+public class NodeHealthTest extends SolrCloudTestCase {
+
+ /**
+ * A standalone (non-ZooKeeper) Jetty instance used by the legacy-mode
tests. The
+ * {@code @ClassRule} ensures it is shut down after all tests in this class
complete.
+ */
+ @ClassRule public static SolrJettyTestRule standaloneJetty = new
SolrJettyTestRule();
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+configureCluster(1).addConfig("conf",
configset("cloud-minimal")).configure();
+standaloneJetty.startSolr(createTempDir());
+
+CollectionAdminRequest.createCollection(DEFAULT_TEST_COLLECTION_NAME,
"conf", 1, 1)
+.process(cluster.getSolrClient());
+ }
+
+ @Test
+ public void testCloudMode_HealthyNodeReturnsOkStatus() throws Exception {
+final var request = new NodeApi.Healthcheck();
+final var response = request.process(cluster.getSolrClient());
+
+assertNotNull(response);
+assertEquals(OK, response.status);
+assertNull("Expected no error on a healthy node", response.error);
+ }
+
+ @Test
+ public void testCloudMode_RequireHealthyCoresReturnOkWhenAllCoresHealthy()
throws Exception {
+final var request = new NodeApi.Healthcheck();
+request.setRequireHealthyCores(true);
+final var response = request.process(cluster.getSolrClient());
+
+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();
Review Comment:
Isn't tehre another method that sets up a cluster that could have the
correct `waitForAllNodes`? So we don't get surprised?
And yes, we need liniting or more rules in AGents to capture these things.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2959005856
##
solr/core/src/test/org/apache/solr/handler/admin/api/NodeHealthTest.java:
##
@@ -0,0 +1,164 @@
+/*
+ * 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.client.api.model.NodeHealthResponse.NodeStatus.FAILURE;
+import static
org.apache.solr.client.api.model.NodeHealthResponse.NodeStatus.OK;
+
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.NodeApi;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.apache.solr.util.SolrJettyTestRule;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+public class NodeHealthTest extends SolrCloudTestCase {
+
+ /**
+ * A standalone (non-ZooKeeper) Jetty instance used by the legacy-mode
tests. The
+ * {@code @ClassRule} ensures it is shut down after all tests in this class
complete.
+ */
+ @ClassRule public static SolrJettyTestRule standaloneJetty = new
SolrJettyTestRule();
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+configureCluster(1).addConfig("conf",
configset("cloud-minimal")).configure();
+standaloneJetty.startSolr(createTempDir());
+
+CollectionAdminRequest.createCollection(DEFAULT_TEST_COLLECTION_NAME,
"conf", 1, 1)
+.process(cluster.getSolrClient());
+ }
+
+ @Test
+ public void testCloudMode_HealthyNodeReturnsOkStatus() throws Exception {
+final var request = new NodeApi.Healthcheck();
+final var response = request.process(cluster.getSolrClient());
+
+assertNotNull(response);
+assertEquals(OK, response.status);
+assertNull("Expected no error on a healthy node", response.error);
+ }
+
+ @Test
+ public void testCloudMode_RequireHealthyCoresReturnOkWhenAllCoresHealthy()
throws Exception {
+final var request = new NodeApi.Healthcheck();
+request.setRequireHealthyCores(true);
+final var response = request.process(cluster.getSolrClient());
+
+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 (SolrClient nodeClient = newJetty.newClient()) {
+ // Sanity check: the new node should start out healthy
+ assertEquals(OK, new NodeApi.Healthcheck().process(nodeClient).status);
+
+ // Break the ZK connection to put the node into an unhealthy state
+ newJetty.getCoreContainer().getZkController().getZkClient().close();
+
+ SolrException e =
+ assertThrows(SolrException.class, () -> new
NodeApi.Healthcheck().process(nodeClient));
+ assertEquals(ErrorCode.SERVICE_UNAVAILABLE.code, e.code());
+ assertTrue(
+ "Expected 'Host Unavailable' in exception message",
+ e.getMessage().contains("Host Unavailable"));
+} finally {
+ newJetty.stop();
+}
+ }
+
+ /**
+ * Verifies that when the node's name is absent from ZooKeeper's live-nodes
set (while the ZK
+ * session itself is still connected), the v2 health-check API throws a
{@code
+ * SERVICE_UNAVAILABLE} exception with a message identifying the live-nodes
check as the cause.
+ *
+ * This specifically exercises the code path at
NodeHealth#getClusterState() that checks {@code
+ * clusterState.getLiveNodes().contains(nodeName)}.
+ */
+ @Test
+ public void testCloudMode_NotInLiveNodes_ThrowsServiceUnavailable() throws
Exception {
+JettySolrRunner newJetty = cluster.startJettySolrRunner();
+try (SolrClient nodeClient = newJetty.newClient()) {
+ // Sanity check: the new node should start out healthy
+ asse
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2917691210
##
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}.
+ *
+ * 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:
done
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2917689589
##
solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealth.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.
*
* This API (GET /v2/node/health) is analogous to the v1 /admin/info/health.
+ *
+ * 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:
i think I've answered both of these
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2917675707
##
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.
*
* This API (GET /v2/node/health) is analogous to the v1 /admin/info/health.
+ *
+ * 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:
okay, resolving now that we moved the biz logic.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2917615362
##
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.
*
* This API (GET /v2/node/health) is analogous to the v1 /admin/info/health.
+ *
+ * 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:
Lots of files are in `.api`..I suspect in a few month we could reorg all
the packages once we get on v2 jax-rs for everything!
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2917575692
##
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.
*
* This API (GET /v2/node/health) is analogous to the v1 /admin/info/health.
+ *
+ * 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:
I actually really like this suggestion, becasue I thought the cAsE of the
name was meaningful, and I was a bit werieded out by it. While we are at it,
do you like it being in org.apache.solr.handler.api? I wonder if we strike the
`.api` sub folder?
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-4029887808 Oh, and the reason, based on spelunking, why we don't support `maxGenerationLag` in v2 is that it's a feature of the old leader/follower capability only. See `TestHealthCheckHandlerLegacyMode`. -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-4027793012 > > Okay, at this point down to a decision on Enum impact, migrating all the business logic out of HealthCheckHandler, and Javadocs and then this may be good to go! > > Chatted with @gerlowskija on phone, and we're going to take a stab at migrating business logic. Javadocs we won't do anything more creative than what we have for now. Forgot to ask about Enum. Got a fix for handling the Enum comparisoin. I migrated the business logic, and removed a duplicate test in the process... @gerlowskija I think this is ready for final review! -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-4025945701 > Okay, at this point down to a decision on Enum impact, migrating all the business logic out of HealthCheckHandler, and Javadocs and then this may be good to go! Chatted with @gerlowskija on phone, and we're going to take a stab at migrating business logic. Javadocs we won't do anything more creative than what we have for now. Forgot to ask about Enum. -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-4025238003 Okay, at this point down to a decision on Enum impact, migrating all the business logic out of HealthCheckHandler, and Javadocs and then this may be good to go! -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2906683220
##
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.
+ *
+ * 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:
I added something, and one thing is we may want to collapse our tests from
`HealthCheckHandlerTest` at some point into this test case.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2906677863
##
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.
+ *
+ * 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:
done!
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2906580309
##
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:
Good questionI think we need to decide what the pattern should be
for these new apis...Where DO we want these apis to link to and be
documented? I don't have a strong opinion, and honestlhy, if we punted this to
a future JIRA I could live with that too.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2906507037
##
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:
Okay, changign this to a enum appears to have some knock on effects. Worth
it? We can just require you to use the SolrJ classes?
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2906468544
##
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:
Okay, using an Enum.I now get this in an existing test... Is this
expected?
https://github.com/user-attachments/assets/27233dbb-69ef-4067-9d41-667c5612ee84";
/>
It all works when you use the generate solrj class.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2906344778
##
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:
good call.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2906349881
##
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.
+ *
+ * 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:
Dunno...Do you think we should ahve TWO tests? The setup is only done
one in beforeClass.. not per test.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2906102936
##
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:
Okay, I think I figured out how to do an Enum...
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2906029307
##
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,
-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:
Ahh... I like it. Honeslty, I was taking a pretty mechanistic approach,
versus really thinking through semantics since my goal was more about getting
rid of our old way of doing v2 versus "What should our API be like?".
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171: URL: https://github.com/apache/solr/pull/4171#discussion_r2905824510 ## 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: Sure! -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2900082778
##
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.
*
* This API (GET /v2/node/health) is analogous to the v1 /admin/info/health.
+ *
+ * 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:
I was thinking that I wanted to
Minimize the churn in existing code, so my thought was to actually leave the
v1 code alone as much as possible and reduce the risk of breaking the business
logic in the v1 handlers. Then when we retire v1 we move the logic into the
v2 versus delegate to v1. If you think we just deal with it now, I just think
it may lead to more code to review!!!
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-4016938499 @gerlowskija all really good comments and also highlights how little I know about the capabilities available in Jax rs and our implementation!!! Will update on Monday. -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
gerlowskija commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2885688419
##
solr/api/src/java/org/apache/solr/client/api/model/NodeHealthResponse.java:
##
@@ -0,0 +1,30 @@
+/*
+ * 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.client.api.model;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/** Response body for the '/api/node/health' endpoint. */
+public class NodeHealthResponse extends SolrJerseyResponse {
+
+ @JsonProperty public String status;
+
+ @JsonProperty public String message;
+
+ @JsonProperty("num_cores_unhealthy")
+ public Long numCoresUnhealthy;
Review Comment:
> I wasn't arguing for a primitive
Ah, I missed that, fair enough. I'm ambivalent on whether we go with
`Integer` or `Long` in this case 🤷
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
dsmiley commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2879765722
##
solr/api/src/java/org/apache/solr/client/api/model/NodeHealthResponse.java:
##
@@ -0,0 +1,30 @@
+/*
+ * 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.client.api.model;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/** Response body for the '/api/node/health' endpoint. */
+public class NodeHealthResponse extends SolrJerseyResponse {
+
+ @JsonProperty public String status;
+
+ @JsonProperty public String message;
+
+ @JsonProperty("num_cores_unhealthy")
+ public Long numCoresUnhealthy;
Review Comment:
Stream.count() returns a long. It doesn't mean that this particular stream
would ever actually need a long.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
epugh commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2879596908
##
solr/api/src/java/org/apache/solr/client/api/model/NodeHealthResponse.java:
##
@@ -0,0 +1,30 @@
+/*
+ * 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.client.api.model;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/** Response body for the '/api/node/health' endpoint. */
+public class NodeHealthResponse extends SolrJerseyResponse {
+
+ @JsonProperty public String status;
+
+ @JsonProperty public String message;
+
+ @JsonProperty("num_cores_unhealthy")
+ public Long numCoresUnhealthy;
Review Comment:
what about the use of `return Math.toIntExact`? Makes me start to think
that whatever else is returning longs should be converted to ints too?
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
dsmiley commented on PR #4171: URL: https://github.com/apache/solr/pull/4171#issuecomment-3991728561 Just want to give kudos to Jason's thoroughly amazing code review. Really shows how important it is that we have the right reviewers on the right issues. Hopefully after this issue is done, similar issues can follow the same lessons/advise. Assuming LLM is doing the bulk of the work, it can be pointed specifically at this PR to learn the key aspects to successfully do similar transitions. -- 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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
dsmiley commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2878851711
##
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.
+ *
+ * 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:
OMG... remove this comment
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
dsmiley commented on code in PR #4171:
URL: https://github.com/apache/solr/pull/4171#discussion_r2878844079
##
solr/api/src/java/org/apache/solr/client/api/model/NodeHealthResponse.java:
##
@@ -0,0 +1,30 @@
+/*
+ * 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.client.api.model;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/** Response body for the '/api/node/health' endpoint. */
+public class NodeHealthResponse extends SolrJerseyResponse {
+
+ @JsonProperty public String status;
+
+ @JsonProperty public String message;
+
+ @JsonProperty("num_cores_unhealthy")
+ public Long numCoresUnhealthy;
Review Comment:
My only point was to use `Integer` and not `Long`. I wasn't arguing for a
primitive or anything else.
--
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]
Re: [PR] SOLR-16458: Migrate NodeHealthAPI from homegrown @EndPoint to JAX-RS [solr]
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.
+ *
+ * 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:
