Skip to content

[ZEPPELIN-6574] Add read-only REST API for interpreter process status - #5403

Open
hyunw9 wants to merge 2 commits into
apache:masterfrom
hyunw9:ZEPPELIN-6574
Open

[ZEPPELIN-6574] Add read-only REST API for interpreter process status#5403
hyunw9 wants to merge 2 commits into
apache:masterfrom
hyunw9:ZEPPELIN-6574

Conversation

@hyunw9

@hyunw9 hyunw9 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

What is this PR for?

Zeppelin server owns interpreter processes but exposes no API to see which ones are running. This adds a read-only snapshot endpoint:

GET /api/interpreter/status

[{
  "settingId": "spark", "settingName": "spark", "groupId": "spark-shared_process",
  "numSessions": 2, "started": true, "host": "127.0.0.1", "port": 51037,
  "startTime": "2026-08-09 14:20:11", "uptimeSeconds": 3184, "errorMessage": null
}]

Review point:

  • No remote probe. Built from in-memory server state only, so a stuck interpreter cannot block the call. started means a process handle exists, not that the process is reachable. A bounded liveness probe is deliberately left to ZEPPELIN-6576.

First sub-task of ZEPPELIN-6568 (server-side status and idle lifecycle for interpreter processes); the remaining sub-tasks build on this.

What type of PR is it?

Feature

Todos

  • - Add InterpreterProcessStatus, a snapshot DTO built from a ManagedInterpreterGroup
  • - Add InterpreterSettingManager#getInterpreterProcessStatuses to aggregate all running groups
  • - Add RemoteInterpreterProcess#getStartTimeMs so uptime is computed without a remote call
  • - Add the GET /api/interpreter/status endpoint
  • - Add unit and REST tests

What is the Jira issue?

How should this be tested?

Included tests, both passing on current master:

  • InterpreterSettingManagerTest#testGetInterpreterProcessStatuses - aggregation is empty before any group exists; after a session is created it reports the setting name, session count, and started=false / port=-1 for an unlaunched process. (13/13)
  • InterpreterRestApiTest#testGetInterpreterProcessStatus - endpoint returns a JSON array. (12/12)

Manually: run a paragraph, then curl -u <admin> http://localhost:8080/api/interpreter/status and confirm the interpreter appears with started: true. Restarting it from the setting page resets uptimeSeconds.

Screenshots (if appropriate)

N/A - REST only.

Questions:

  • Does the license files need to update? No.
  • Is there breaking changes for older versions? No - additive and read-only.
  • Does this needs documentation? Can add a REST API doc section here if preferred, otherwise once the sibling sub-tasks land.

@tbonelee tbonelee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the response shape against the example in the description. JsonResponse does not enable serializeNulls (JsonResponse:96-101) and JsonExclusionStrategy skips nothing, so Gson's default applies. Serializing one group with no process gives:

{"status":"OK","message":"","body":[{"settingId":"2ABCDEFG","settingName":"test","groupId":"test-shared_process","numSessions":2,"started":false,"port":-1,"uptimeSeconds":0}]}

Two differences from the description:

  • Null fields are omitted entirely rather than serialized as null, so "errorMessage": null does not appear. A consumer could reasonably be unsure whether an absent key means null, so it may help to match the example to the real output or to state the rule explicitly.
  • The real response is wrapped as {status, message, body:[...]} rather than a bare array. The code looks right and the description looks out of date here, and the test already reads body.

On the documentation question: docs/usage/rest_api/interpreter.md already exists, so a section there seems like the natural home. If the sibling sub-tasks are going to add fields, writing it once at the end may be easier, so it depends on how you have the series planned.

A few more notes inline. Thanks for splitting this out as a read-only first step, the scope reads well.

Comment on lines +47 to +48
this.started = process != null;
if (started) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ManagedInterpreterGroup.getOrCreateInterpreterProcess() assigns remoteInterpreterProcess = createInterpreterProcess(...) before start(), and the reader does not take interpreterProcessCreationLock. A call during a launch can therefore observe a handle whose host and port are still at their initial values (null / -1 at RemoteInterpreterManagedProcess:35-36).

A single started boolean does not let a consumer tell that window apart from a fully started process, so you may want a second field. Conveniently ManagedInterpreterGroup.isLaunchingInterpreterProcess() already exists, or the state could be derived from whether port has been filled in. The latter looks more robust for separating "handle created / awaiting registration / registered" at no extra cost, though you may prefer the simplicity of one boolean, so I will leave it as a matter of taste.

Combined with the uptime in the note above, this would also make "stuck awaiting registration" visible on its own, which catches a fair amount without any remote probe.

Comment on lines +49 to +50
this.host = process.getHost();
this.port = process.getPort();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the values this snapshot reads are non-volatile and are written on a different thread from the one reading them:

  • ManagedInterpreterGroup.remoteInterpreterProcess: written inside a synchronized block, read without the lock
  • RemoteInterpreterManagedProcess.host / port: written by the Thrift registration callback, read by the REST thread
  • errorMessage: written by the YarnAppMonitor scheduler thread or the K8s path, read by the REST thread

All of this predates the PR, so it is not something this change introduced. I mention it because this API is the first place that state becomes a documented contract, so reporting a stale value now has a visible consequence. It also feeds directly into the phase decision if you take the port-based approach above.

A few volatile modifiers look close to free here, but if that feels out of scope it seems fine as follow-up. Your call.

this.host = process.getHost();
this.port = process.getPort();
this.startTime = process.getStartTime();
this.uptimeSeconds = (System.currentTimeMillis() - process.getStartTimeMs()) / 1000;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startTimeMs is stamped in the RemoteInterpreterProcess constructor, and RemoteInterpreterRunningProcess is constructed fresh in two places:

  • RecoveryUtils:94 (reconnecting to a process that survived a server restart)
  • StandardInterpreterLauncher:59 (connecting to an already running interpreter)

In those cases uptimeSeconds measures time since the server created the handle rather than process uptime, so right after recovery a Spark process that has been up for hours would report an uptime of a few seconds.

The existing getStartTime() string has the same limitation, so this is not new, but the name uptimeSeconds reads as process lifetime and may be easier to misinterpret. You have been upfront about the limits of started in the Javadoc, and one option is to do the same here, or to rename the field to reflect the server's point of view. Which is better probably depends on how you plan to use the value, so I will leave the call to you.

One small thing alongside it: startTime and startTimeMs each call new Date() / System.currentTimeMillis() independently. Deriving startTime from startTimeMs would keep the two consistent.

this.port = process.getPort();
this.startTime = process.getStartTime();
this.uptimeSeconds = (System.currentTimeMillis() - process.getStartTimeMs()) / 1000;
this.errorMessage = process.getErrorMessage();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR describes "No remote probe. Built from in-memory server state only, so a stuck interpreter cannot block the call" as the core contract of this endpoint, but this line may not hold to it. Two separate things seem to be going on.

First, getErrorMessage() is a synchronous remote call in some launchers:

  • K8sRemoteInterpreterProcess:525 -> getPodPhase() -> client.pods()...get() (a call to the API server)
  • DockerInterpreterProcess:495 -> client.inspectContainer() (a call to the daemon)

errorMessage is only populated when started == true, so this fires precisely in the healthy case, once per group, serially. If the API server is slow the whole endpoint waits on it, which looks like the bounded liveness probe that was deferred to ZEPPELIN-6576 leaking into this PR.

Second, on the default launcher this value may not be an error signal at all. ProcessLauncher:139 reads:

if (!StringUtils.isBlank(processOutput.getProcessExecutionOutput())) {
  return processOutput.getProcessExecutionOutput();
}

and stopCatchLaunchOutput() only stops appending, it does not clear the launchOutput buffer (ProcessLauncher:180, ExecRemoteInterpreterProcess:215). So a healthy interpreter keeps a non-null errorMessage for as long as its launch output is retained, and for something like Spark that string would be carried in the JSON on every poll, once per group. Please correct me if I have misread this path.

Dropping the field would remove both the blocking call and the payload concern, and would make the contract in the PR description true on every launcher. Adding diagnostics later, alongside the bounded probe in 6576, may be a more natural fit. What do you think?

If you would rather address it here, there is another option. The failure information is already pushed onto the process object:

  • RemoteInterpreterManagedProcess:101, processStopped(String)
  • callers: YarnAppMonitor:81 (a background thread polls YARN and pushes the diagnostics), K8sRemoteInterpreterProcess:169/183/191/211

The cost of finding out has therefore already been paid by a watcher, and a reader only needs to read a field. Holding that value in the base class behind a final accessor that a launcher cannot override would let the snapshot take the cheap path while getErrorMessage() keeps its current behaviour for paragraph error reporting.

That direction has limits worth stating too. Docker records nothing at all (no watcher, no processStopped call), and K8s only records failures during start() and stop(), so a pod that dies after starting is not captured. Closing those gaps is follow-up work in any case, and when it comes up it might be worth considering whether the answer is a probe on the read path or having each launcher detect the death and push it, the way YarnAppMonitor already does and the way the existing PodPhaseWatcher could if it were kept for the pod's lifetime. Perhaps one for the 6576 discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants