[SPARK-57941][CONNECT] Expose Spark Connect sessions and operations via REST API#57016
[SPARK-57941][CONNECT] Expose Spark Connect sessions and operations via REST API#57016venkata91 wants to merge 1 commit into
Conversation
|
cc @yaooqinn @pan3793 @dongjoon-hyun @cloud-fan @HyukjinKwon for review. REST API for the Spark Connect server, mirroring the SQL REST API pattern. It reads from the same KVStore that backs the Connect UI tab and should work with the History Server as well. Would appreciate your review when you have a chance. Thanks! |
sunchao
left a comment
There was a problem hiding this comment.
Summary
Prior state and problem
Spark Connect already records session and operation history for the Connect UI, but operators cannot consume the same data through Spark's /api/v1 interface. This PR fills that gap for both live applications and History Server playback.
Design approach
The change follows the existing SQL status API pattern: Jersey discovers a new Connect resource package, the resource reads the existing Connect KVStore, and public DTOs provide JSON responses. It adds application and application-attempt routes without creating a second listener or collection path.
Correctness / compatibility analysis
The application scoping, view authorization, KVStore lookups, 404 behavior, history reconstruction, duration calculations, deterministic collection serialization, and internal visibility changes are consistent with the surrounding Spark status APIs. I found one P2 in the operation-detail route: jobTag is not guaranteed to be a safe URL path segment, so some operations returned by the list endpoint cannot be fetched individually. The inline comment has the triggering case and suggested fix.
The new DTOs intentionally expose the existing Connect UI/history representation, including millisecond timestamps, 0 sentinels for unfinished events, and CLOSED as the final query-history state. These choices do not break an existing API, but they will become part of the public wire contract.
Key design decisions
- Reuse
SparkConnectServerAppStatusStoreinstead of duplicating state. - Discover resources recursively under
org.apache.spark.status.api.v1, matching SQL and Streaming. - Keep listener/store classes internal by widening them only to
private[spark]. - Return statements, lifecycle state, tags, job IDs, SQL execution IDs, and error details in the operation DTO.
Implementation sketch
ApiConnectRootResource supplies live and attempt-aware routes. ConnectResource maps SessionInfo and ExecutionInfo into SessionData and ExecutionData, while BaseAppResource supplies application lookup and UI authorization. The tests cover field mapping, live HTTP discovery, list/detail retrieval, and missing-ID responses.
Behavioral changes worth calling out
This adds four read-only endpoint families under /applications/[app-id]/connect, plus attempt variants. The responses expose the same redacted statement and error information already available to authorized Connect UI viewers. Existing endpoint behavior and data collection are unchanged.
Suggested improvements
Please address the inline P2 before merging. As optional API hardening, consider adding SQL-style offset / length and reduced-detail controls for operation lists, and adding explicit History Server replay coverage.
Upstream checks are green. The focused mapping suite passed locally; the live-server suite could not initialize under this machine's unsupported JDK 26 because jdk.internal.ref.Cleaner is unavailable, which is an environment limitation rather than a PR failure.
| } | ||
|
|
||
| @GET | ||
| @Path("operations/{jobTag}") |
There was a problem hiding this comment.
[P2] Keep opaque job tags out of a single path segment
jobTag embeds the raw Connect userId, and valid user IDs may contain /. Such an operation appears in /operations, but its returned tag cannot be used with this endpoint: a literal slash creates another route segment, while encoding it as %2F is rejected by Spark's default Jetty 12.1 URI compliance as AMBIGUOUS_PATH_SEPARATOR. Please use a path-safe lookup key (for example operationId) or accept the opaque tag as a query parameter, and add regression coverage with a user ID such as tenant/alice.
There was a problem hiding this comment.
Good catch, thanks — fixed. The operation-detail route now keys on operationId (a validated UUID, so always path-safe) instead of the jobTag:
- Added a secondary
@KVIndex("operationId")onExecutionInfoand agetExecutionByOperationIdlookup on the store. GET .../connect/operations/{operationId}now looks up by that index;jobTagis still returned in the DTO for reference.- Regression test added with
userId = "tenant/alice"(ConnectResourceWithActualDataSuite): it drives a real query, confirms the returnedjobTagcontains the raw/, and that the operation is still fetchable via itsoperationId. A store-level test inConnectResourceSuiteexercises the new index directly (also covering the History Server replay path, which uses the sameSparkConnectServerAppStatusStore).
Implementation note: I used a method-based @JsonIgnore @KVIndex("operationId") def rather than annotating the constructor val, because Scala targets a param-val annotation at the parameter (not the field/getter), so the KVStore reflection doesn't pick it up — the tests caught that.
f361a26 to
8629e17
Compare
|
Thanks for the thorough review, @sunchao! Addressed in the latest push (
All connect REST suites pass locally on the current master (rebased). The DTO wire-contract fields you flagged (ms timestamps, |
|
@sunchao Addressed your review comments. Please take a look again whenever you get a chance. Looks like the failing test is unrelated and flaky. Not sure how to make that green though :) |
| duration = info.totalTime(info.closeTimestamp), | ||
| executionTime = info.totalTime(info.finishTimestamp), | ||
| sparkSessionTags = info.sparkSessionTags.toSeq.sorted, | ||
| jobIds = info.jobId.toSeq.sorted, |
There was a problem hiding this comment.
jobId/sqlExecId are numeric ids stored as strings, so .sorted is lexicographic: ["2","10"] returns as ["10","2"]. The unit test only passes because "1","2" and "0","10" sort the same both ways. Use .sortBy(_.toInt) (both lines), or document the order is intentionally lexicographic.
| } | ||
|
|
||
| private def paginate[T](list: Seq[T], offset: Int, length: Int): Seq[T] = { | ||
| if (length < 0) list else list.slice(offset, offset + length) |
There was a problem hiding this comment.
This slices after the caller has already materialized the whole store getSessionList/getExecutionList (lines 35 and 54) call KVUtils.viewToSeq(store.view(...)), loading all N rows on every request. So ?offset=0&length=10 still walks the entire store, and the default length=-1 (lines 33 and 52) means a bare GET /connect/operations returns an unbounded payload an OOM/latency risk on a long-lived server. Please push offset/length into the KVStoreView (skip/max) and cap the default length.
There was a problem hiding this comment.
[P3] There is also a concrete correctness case for moving this pagination into the KVStore view: offset + length is evaluated as an Int, so ?offset=1&length=2147483647 wraps the end index to Int.MinValue and returns an empty page even when rows remain. Using skip(offset).max(length) as suggested above avoids the overflow; please cover this boundary as well.
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed the current head after the latest fix. The original jobTag path-segment issue is fixed. I found two remaining P2s inline.
Non-blocking: the PR description still lists /operations/[job-tag] and four tests; the implementation now uses operationId and contains six tests.
| .index("operationId") | ||
| .first(operationId) | ||
| .last(operationId)) | ||
| .headOption |
There was a problem hiding this comment.
[P2] Keep the full operation identity in this lookup
operationId is only unique within an execution key: the manager keys operations by (userId, sessionId, operationId), and clients may supply any valid UUID. Two sessions can therefore legally create distinct executions with the same UUID. Both rows are returned by this secondary-index range, but headOption silently selects one, so following the ID from the other /operations list entry returns the wrong execution. Please include enough user/session identity in the route and lookup (or accept the unique jobTag as a query parameter), and add a duplicate-operation-ID-across-sessions test.
| // Secondary index so an execution can be looked up by its operation id. The natural key (the | ||
| // job tag) embeds the raw user id and is not a safe URL path segment, whereas the operation id | ||
| // is always a UUID. See SparkConnectServerAppStatusStore.getExecutionByOperationId. | ||
| @JsonIgnore @KVIndex("operationId") |
There was a problem hiding this comment.
[P2] Invalidate disk stores that lack this index
Existing disk-backed History Server stores were written without operationId index entries. Since AppStatusStore.CURRENT_VERSION is unchanged, loadDiskStore reopens them without replay, while LevelDB and RocksDB populate secondary indexes only on writes and do not backfill an added index on open. The operation list still reads the old natural records, but every detail lookup for those cached applications returns 404 until the store is rebuilt. Please bump the store version or add migration/fallback behavior, with an old-store reopen test.
8629e17 to
1b4e27d
Compare
What changes were proposed in this pull request?
Spark Connect server session and operation (execution) data is collected by
SparkConnectServerListenerand shown in the "Connect" Spark UI tab, but unlike SQL (SPARK-27142) and Streaming, it is not reachable through the/api/v1REST API.This adds a REST resource mirroring the SQL REST API. New endpoints:
GET /applications/[app-id]/connect/sessions— list sessionsGET /applications/[app-id]/connect/sessions/[session-id]— one sessionGET /applications/[app-id]/connect/operations— list operationsGET /applications/[app-id]/connect/operations/[job-tag]— one operation(and the
[attempt-id]variants).Implementation notes for reviewers:
org.apache.spark.status.api.v1.connect, so they are auto-discovered by the existing Jersey package scan (ApiRootResourceregistersorg.apache.spark.status.api.v1recursively) exactly like the SQL (...v1.sql) and Streaming (...v1.streaming) resources. No wiring changes are needed.ConnectResourcereads fromui.store.store— the sameKVStorethat backs the Connect UI tab — via the existingSparkConnectServerAppStatusStore. It adds no new data collection.SessionInfo,ExecutionInfoandExecutionStateare widened fromprivate[connect]toprivate[spark]so the resource package can read them. They remain internal (not public API).SessionData/ExecutionDataDTOs inapi.scala, following the SQL/Streaming convention (no@sinceon these status DTOs).Because the Connect listener already persists its events to the event log, these endpoints also work in the History Server through the existing
SparkConnectServerHistoryServerPlugin.Why are the changes needed?
The Connect UI tab exposes per-session and per-operation information (user, timings, state, job/SQL ids, errors) that is only reachable by scraping HTML. Jobs, stages, SQL and Streaming are all queryable as JSON under
/api/v1; Spark Connect should be too, so operators can build monitoring and automation on top of the Connect server the same way they do for the rest of Spark.Does this PR introduce any user-facing change?
Yes. It adds new read-only REST endpoints under
/api/v1/applications/[app-id]/connect/...(listed above) and documents them indocs/monitoring.md. There is no change to existing endpoints or behavior.How was this patch tested?
New tests added, run with
build/sbt 'connect/testOnly org.apache.spark.status.api.v1.connect.*':ConnectResourceSuite— unit tests for theSessionData/ExecutionDatamapping (exercises the privateprepare*methods directly, mirroringSqlResourceSuite).ConnectResourceWithActualDataSuite— end-to-end test that boots the realSparkConnectServicewith the UI enabled, runs a query through a Connect session so the listener populates the KVStore, then hits the endpoints over HTTP and asserts on the JSON (including 404s for unknown ids).Manual verification against a running Connect server: started the server, ran
spark.sql("select 1 + 1")from a Connect client, and confirmed the endpoints return the expected JSON:scalafmt check passes for the connect module (
./build/mvn scalafmt:format -Dscalafmt.validateOnly=true -pl sql/connect/server→BUILD SUCCESS).Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)