You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "big-cir (via GitHub)" <gi...@apache.org> on 2026/08/13 15:54:06 UTC
[PR] [ZEPPELIN-6576] Add REST API for a bounded on-demand interpreter health check [zeppelin]
big-cir opened a new pull request, #5419:
URL: https://github.com/apache/zeppelin/pull/5419
### What is this PR for?
After restarting an interpreter from the setting page there is no way to ask whether it is reachable. The badge on that page is the dependency download status (`InterpreterSetting.Status`), which the code that starts and stops processes never touches, so it stays green whether the process is gone, stuck, or was never started. The only remaining way to find out is to run a paragraph, and for Spark that means the check itself takes cluster resources.
This adds a probe that a user asks for explicitly:
```
POST /api/interpreter/setting/{settingId}/healthcheck
{
"settingId": "2CH2VMY7B",
"settingName": "spark",
"groups": [
{ "groupId": "2CH2VMY7B-shared_process", "healthy": true, "reason": "OK",
"alive": true, "running": true, "probeTookMs": 3 },
{ "groupId": "2CH2VMY7B-user2", "healthy": false, "reason": "NOT_REACHABLE",
"alive": true, "running": false, "probeTookMs": 1004 },
{ "groupId": "2CH2VMY7B-user3", "healthy": false, "reason": "PROBE_TIMEOUT",
"probeTookMs": 3000 }
]
}
```
Review points:
- **The deadline has to come from the caller.** `isRunning()` takes no timeout, and its cost is not uniform: local and yarn read memory, docker opens a socket with a hardcoded one second timeout, and the k8s launcher asks the kube-apiserver through a client built with library defaults (10s per request, up to 10 retries with backoff). The thrift path is worse, since the socket is created without a timeout and the call is retried three times. So the probes run on another thread, and the request stops waiting after a budget shared by the whole setting. That budget is a 3 second constant. The number itself is arbitrary, so please suggest a more meaningful one and I will change it.
- **Bounding the response does not reclaim the probe.** Cancelling a future only interrupts, and neither a blocking socket connect nor an HTTP client call ends on an interrupt, so a probe thread can stay busy after its result was given up on. The pool is therefore limited to 4 daemon threads. Daemon matters because normal shutdown does not call `System.exit`, so a stuck non daemon thread would delay it.
- **It never starts an interpreter.** Restart only closes the processes and the next use creates them again, so a setting with nothing running is reported with the reason `NOT_RUNNING` rather than as a failure. The frontend sub-tasks need that distinction to render something other than a red badge.
- **One setting can own many processes.** An isolated setting has a group per user or note, so the response carries an entry per group. Probes are submitted together and collected against the one deadline, so an interpreter that does not answer costs the request its budget once instead of delaying the groups that would have answered right away.
- **Two places where this differs from the ticket, both deliberate.** The ticket names `isAlive`, but the interface documents `isRunning` as "the interpreter can communicate with server" while `isAlive` only says a process exists, so `healthy` follows `isRunning` and both values are reported. The ticket also names two outcomes, healthy and unhealthy, which the response keeps as the `healthy` flag; the `reason` next to it is what lets a caller tell a setting that was never started from one that timed out. Happy to reduce it if you prefer the literal reading.
- **Known limit.** For the local and yarn launchers, `isRunning()` reads in process state, so an interpreter that is up but no longer answering is still reported healthy there. Only docker, k8s, and existing process check from outside. A thrift level ping would answer that properly, but it needs a schema change and has no bound of its own, so it is left out.
Out of scope, on purpose: `GET /api/session` calls `isRunning()` per session on a timer and has the same cost problem, but fixing it is a separate change.
### What type of PR is it?
Feature
### Todos
* [x] - Add `InterpreterHealthCheck`, the response model, built through factory methods so `healthy` cannot disagree with `reason`
* [x] - Add `InterpreterHealthChecker`, which probes the groups of a setting under one deadline
* [x] - Add the `POST /api/interpreter/setting/{settingId}/healthcheck` endpoint with the note based authorization that restart already uses
* [x] - Add unit and REST tests
* [x] - Document the endpoint in `docs/usage/rest_api/interpreter.md`
### What is the Jira issue?
* [ZEPPELIN-6576](https://issues.apache.org/jira/browse/ZEPPELIN-6576)
### How should this be tested?
Included tests, all passing on current master:
```
export JAVA_HOME=<your JDK 11 home>
./mvnw package -pl zeppelin-server --am \
-Dtest=InterpreterHealthCheckerTest,InterpreterRestApiTest -DfailIfNoTests=false
```
- `InterpreterHealthCheckerTest`, 7 tests. Covers the deadline, a slow group not hiding a fast one, group order, not probing a launching group, not starting an interpreter to answer, and a single remote call in the healthy case. (7 passed, 0 failed)
- `InterpreterRestApiTest`, 2 added. A health check of an unused setting returns `NOT_RUNNING` with no groups and leaves the setting without one, and an unknown setting returns 404. (13 passed, 0 failed)
License check: `./mvnw clean org.apache.rat:apache-rat-plugin:check -Prat -pl zeppelin-server` reports 0 unapproved.
Manually, on a running server:
```
curl -u <admin> -X POST http://localhost:8080/api/interpreter/setting/<settingId>/healthcheck
```
Right after a restart this answers `NOT_RUNNING` without starting anything. Run a paragraph first and the same call reports the group as healthy.
### Screenshots (if appropriate)
N/A, REST only.
### Questions:
* Does the license files need to update? No. The four new files carry the ASF header and the RAT check reports no unapproved files.
* Is there breaking changes for older versions? No. The endpoint is new and nothing existing changes behavior.
* Does this needs documentation? Yes, and it is included in `docs/usage/rest_api/interpreter.md`.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6576] Add REST API for a bounded on-demand interpreter health check [zeppelin]
Posted by "big-cir (via GitHub)" <gi...@apache.org>.
big-cir commented on code in PR #5419:
URL: https://github.com/apache/zeppelin/pull/5419#discussion_r4375071373
##########
docs/usage/rest_api/interpreter.md:
##########
@@ -520,18 +520,120 @@ The role of registered interpreters, settings and interpreters group are describ
</tr>
</table>
+<br/>
+### Health check an interpreter
Review Comment:
Agreed, and it applies beyond Kubernetes: no launcher's check gets a reply from the interpreter service.
- The docs now define `healthy` / `OK` as the launcher reporting the interpreter as running, and list per launcher what is actually checked.
- Groups that were checked now carry `checkMethod` (e.g. `KUBERNETES_POD_PHASE`, `TCP_CONNECT`), so a caller can see what `OK` is based on.
I kept the `healthy` / `OK` names to match the ticket, but the docs no longer present `OK` as confirmed reachability.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6576] Add REST API for a bounded on-demand interpreter health check [zeppelin]
Posted by "big-cir (via GitHub)" <gi...@apache.org>.
big-cir commented on code in PR #5419:
URL: https://github.com/apache/zeppelin/pull/5419#discussion_r4375068381
##########
zeppelin-server/src/main/java/org/apache/zeppelin/interpreter/InterpreterHealthChecker.java:
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.zeppelin.interpreter;
+
+import com.google.common.annotations.VisibleForTesting;
+
+import org.apache.zeppelin.interpreter.InterpreterHealthCheck.GroupHealth;
+import org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcess;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Probes the interpreter processes of one interpreter setting and reports whether they answer,
+ * within a deadline that holds for the whole request.
+ *
+ * <p>Unlike the interpreter status snapshot, which is built from server memory alone, this does
+ * contact the interpreter. That is only acceptable because a probe happens when a user explicitly
+ * asks for one, against a single setting, and under a deadline: {@code isRunning()} costs a socket
+ * connect for docker and a kube-apiserver round trip for k8s, so the same call would be an accident
+ * if a listing made it on a timer.
+ *
+ * <p>The deadline can only be imposed from the outside. {@code isRunning()} takes no timeout
+ * argument, the socket connect behind the docker implementation hardcodes one second, and the
+ * kubernetes client is created with the defaults of its library. So the probe runs on another
+ * thread and this class stops waiting for it, which bounds the response but not the probe:
+ * {@code Future#cancel(boolean)} interrupts, and neither a blocking socket connect nor an HTTP
+ * client call ends on an interrupt. A probe thread can therefore stay busy after its result was
+ * given up on, which is why the pool is small and its threads are daemons.
+ */
+public class InterpreterHealthChecker {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(InterpreterHealthChecker.class);
+
+ /**
+ * Budget for one health check request, shared by all groups of the setting rather than granted to
+ * each of them, so that an isolated setting cannot turn into a per group multiple of it.
+ */
+ @VisibleForTesting
+ static final long PROBE_TIMEOUT_IN_MILLIS = 3_000;
+
+ /**
+ * A probe that was given up on keeps its thread, so this bounds how many can pile up while an
+ * interpreter host is unreachable. Further probes wait for a thread instead of getting one, and
+ * are reported as timed out by the deadline if they never start, which is the honest answer.
+ */
+ private static final int MAX_CONCURRENT_PROBES = 4;
+
+ private final long probeTimeoutInMillis;
+ private final ExecutorService probeExecutor;
+
+ public InterpreterHealthChecker() {
+ this(PROBE_TIMEOUT_IN_MILLIS);
+ }
+
+ @VisibleForTesting
+ InterpreterHealthChecker(long probeTimeoutInMillis) {
+ this.probeTimeoutInMillis = probeTimeoutInMillis;
+ this.probeExecutor = newProbeExecutor();
+ }
+
+ private static ExecutorService newProbeExecutor() {
+ ThreadPoolExecutor executor = new ThreadPoolExecutor(
+ MAX_CONCURRENT_PROBES, MAX_CONCURRENT_PROBES, 60L, TimeUnit.SECONDS,
Review Comment:
Thanks for reproducing this, you're right.
- The queue is now bounded (64 entries). A probe that doesn't fit is rejected right away with a new reason, `PROBE_UNAVAILABLE`.
- Cancelled probes are purged from the queue before the request returns.
- A probe that never got a thread is also reported as `PROBE_UNAVAILABLE`, without `probeTookMs`. Only a probe that started and ran past the deadline is `PROBE_TIMEOUT`.
The executor is shared across requests, so while all four threads are held, a healthy interpreter still can't be probed and gets `PROBE_UNAVAILABLE`. What the change guarantees is that pending work stays bounded and the reason tells this apart from a probe that timed out. Limiting how long a thread is held would need timeouts on the launcher calls themselves (e.g. the Kubernetes API request), so I'd leave that for a follow-up.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6576] Add REST API for a bounded on-demand interpreter health check [zeppelin]
Posted by "hyunw9 (via GitHub)" <gi...@apache.org>.
hyunw9 commented on code in PR #5419:
URL: https://github.com/apache/zeppelin/pull/5419#discussion_r4305226854
##########
zeppelin-server/src/main/java/org/apache/zeppelin/interpreter/InterpreterHealthChecker.java:
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.zeppelin.interpreter;
+
+import com.google.common.annotations.VisibleForTesting;
+
+import org.apache.zeppelin.interpreter.InterpreterHealthCheck.GroupHealth;
+import org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcess;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Probes the interpreter processes of one interpreter setting and reports whether they answer,
+ * within a deadline that holds for the whole request.
+ *
+ * <p>Unlike the interpreter status snapshot, which is built from server memory alone, this does
+ * contact the interpreter. That is only acceptable because a probe happens when a user explicitly
+ * asks for one, against a single setting, and under a deadline: {@code isRunning()} costs a socket
+ * connect for docker and a kube-apiserver round trip for k8s, so the same call would be an accident
+ * if a listing made it on a timer.
+ *
+ * <p>The deadline can only be imposed from the outside. {@code isRunning()} takes no timeout
+ * argument, the socket connect behind the docker implementation hardcodes one second, and the
+ * kubernetes client is created with the defaults of its library. So the probe runs on another
+ * thread and this class stops waiting for it, which bounds the response but not the probe:
+ * {@code Future#cancel(boolean)} interrupts, and neither a blocking socket connect nor an HTTP
+ * client call ends on an interrupt. A probe thread can therefore stay busy after its result was
+ * given up on, which is why the pool is small and its threads are daemons.
+ */
+public class InterpreterHealthChecker {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(InterpreterHealthChecker.class);
+
+ /**
+ * Budget for one health check request, shared by all groups of the setting rather than granted to
+ * each of them, so that an isolated setting cannot turn into a per group multiple of it.
+ */
+ @VisibleForTesting
+ static final long PROBE_TIMEOUT_IN_MILLIS = 3_000;
+
+ /**
+ * A probe that was given up on keeps its thread, so this bounds how many can pile up while an
+ * interpreter host is unreachable. Further probes wait for a thread instead of getting one, and
+ * are reported as timed out by the deadline if they never start, which is the honest answer.
+ */
+ private static final int MAX_CONCURRENT_PROBES = 4;
+
+ private final long probeTimeoutInMillis;
+ private final ExecutorService probeExecutor;
+
+ public InterpreterHealthChecker() {
+ this(PROBE_TIMEOUT_IN_MILLIS);
+ }
+
+ @VisibleForTesting
+ InterpreterHealthChecker(long probeTimeoutInMillis) {
+ this.probeTimeoutInMillis = probeTimeoutInMillis;
+ this.probeExecutor = newProbeExecutor();
+ }
+
+ private static ExecutorService newProbeExecutor() {
+ ThreadPoolExecutor executor = new ThreadPoolExecutor(
+ MAX_CONCURRENT_PROBES, MAX_CONCURRENT_PROBES, 60L, TimeUnit.SECONDS,
Review Comment:
Following up on the blocking-call concern raised in #5403: the four-thread limit bounds active probes, but the LinkedBlockingQueue is unbounded.
If all four probes ignore interruption, subsequent checks queue and time out without running. Cancelling their futures does not remove those tasks from this executor's queue.
when i reproduced this with the checker implementation, three subsequent requests left three cancelled tasks queued, and the healthy process was never probed.
Could we bound pending work and remove cancelled queued tasks?
##########
docs/usage/rest_api/interpreter.md:
##########
@@ -520,18 +520,120 @@ The role of registered interpreters, settings and interpreters group are describ
</tr>
</table>
+<br/>
+### Health check an interpreter
Review Comment:
Thanks for documenting the local/YARN limitation in the PR description. The same distinction appears relevant to Kubernetes: isRunning() checks the Pod phase and started, rather than a response from the interpreter.
This means healthy=true / reason=OK can be returned without establishing interpreter reachability, while the API documentation describes the result as whether the interpreter answered.
Could we document the launcher-specific guarantees in the API docs and avoid presenting OK as confirmed reachability?
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org