Skip to content

Comments

[SPARK-55606][CONNECT] Server-side implementation of GetStatus API#54445

Open
terana wants to merge 5 commits intoapache:masterfrom
terana:get-status-server
Open

[SPARK-55606][CONNECT] Server-side implementation of GetStatus API#54445
terana wants to merge 5 commits intoapache:masterfrom
terana:get-status-server

Conversation

@terana
Copy link

@terana terana commented Feb 24, 2026

What changes were proposed in this pull request?

Server-side implementation of the GetStatus API:

  • Introduce a variable in ExecuteEventsManager to track execution termination reason after it's closed.
  • Track minimal execution termination information in SessionHolder's inactiveOperations cache.
  • Use SessionHolder's activeOperations and inactiveOperations lists for determining execution status in GetStatus API handler.
  • Add plugin interface for GetStatus operation for processing custom proto extensions.

Why are the changes needed?

GetStatus API allows to monitor status of executions in a session, which is particularly useful in multithreaded clients.

Does this PR introduce any user-facing change?

Yes. It's a new Spark Connect API.

How was this patch tested?

  • New tests were added.
  • E2E tests with checking for real (not mocked) execution lifecycles are coming in client-side PR.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude 4.6 Opus High

.createWithDefault(Nil)

val CONNECT_EXTENSIONS_GET_STATUS_CLASSES =
buildStaticConf("spark.connect.extensions.getStatus.classes")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean a user can configure this from within the session? Is this something we desire?

* the closed state itself does not convey why the operation ended, this value preserves that
* information for later use.
*/
private[connect] def terminationReason: Option[TerminationReason] = _terminationReason
Copy link
Contributor

Choose a reason for hiding this comment

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

What is accessing this? And is this access happening from multiple threads? If so then _terminationReason should be volatile.

case None => Seq.empty
}
} catch {
case e: Throwable =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't catch Throwable, that also covers JVM errors. Please use NonFatal instead.

case None => Seq.empty
}
} catch {
case e: Throwable =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

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

A couple of small nits/questions. Otherwise LGTM.

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