[SPARK-55280][CONNECT] Add GetStatus proto to support execution status monitoring#54062
[SPARK-55280][CONNECT] Add GetStatus proto to support execution status monitoring#54062terana wants to merge 3 commits intoapache:masterfrom
Conversation
JIRA Issue Information=== New Feature SPARK-55280 === This comment was automatically generated by GitHub Actions |
| optional string client_observed_server_side_session_id = 4; | ||
|
|
||
| // The type of status being requested. | ||
| // Implies mutually exlusive status types, e.g. session-level, operation-level, other custom |
There was a problem hiding this comment.
Nit: Typo
| // Implies mutually exlusive status types, e.g. session-level, operation-level, other custom | |
| // Implies mutually exclusive status types, e.g. session-level, operation-level, other custom |
| OPERATION_STATE_UNKNOWN = 1; | ||
| OPERATION_STATE_RUNNING = 2; | ||
| OPERATION_STATE_TERMINATING = 3; | ||
| OPERATION_STATE_TERMINATED = 4; |
There was a problem hiding this comment.
Should we also include cause of termination? (Successfull/Failed/Cancelled)
There was a problem hiding this comment.
With abandoned operations counted as cancelled? Sounds good, will add.
We would need to introduce tracking of the termination reason for closed operations.
| // The type of status being requested. | ||
| // Implies mutually exlusive status types, e.g. session-level, operation-level, other custom | ||
| // stats. | ||
| oneof status_type { |
There was a problem hiding this comment.
I've been thinking about this and I'm conlficted between a top-level oneof here vs a hybrid:
repeated StatusTypeRequest status_types = ...
message StatusTypeRequest {
oneof status_type {
...
}
}The top-level oneof is simple and follows the principle of YAGNI but with the context of this RPC being used for monitoring/polling, serving multiple status types in a single round-trip would help avoid complexity on the client side where it may need to look at several different points of information to proceed.
@hvanhovell WDYT?
There was a problem hiding this comment.
The worst thing that can happen if we make it top-lvl is if request infos are overlapping, and we return inconsistent data in two response types because of a race. That's also true for two separate oneof requests, but less confusing.
Otherwise, oneof is not strictly needed indeed for read-only requests, as they should not interfere much (other than being inconsistent with each other, affecting response times).
There was a problem hiding this comment.
IMHO, there is no need to guarantee consistency by using oneOf to limit the client to reading only one type of data per request. The important thing to consider is how to have the server handle these requests using a consistent snapshot for reading. That said, a hybrid approach is more flexible because it is more expressive and makes it easier to guarantee consistency if the server supports it.
|
If I understand correctly, simply updating a protobuf file and adding the generated code doesn’t change the behavior, so the answer to 'Does this PR introduce any user-facing change?' should be no? |
Initially, this rpc will provide operations statuses. It's designed to be extensible in two ways: - Requesting different status types (operation-level, session-level, some specific stats). - Requesting different information in each type.
terana
force-pushed
the
connect-get-status
branch
from
4143a02 to
ab0696d
Compare
hvanhovell
left a comment
hvanhovell
left a comment
There was a problem hiding this comment.
LGTM - merging to master. Thanks!
4 participants
What changes were proposed in this pull request?
Add GetStatus proto to support execution status monitoring
Initially, this rpc will provide operations statuses. It's designed to be extensible in two ways:
Why are the changes needed?
This API will allow users to monitor status (e.g. RUNNING/TERMINATING/TERMINATED) of the operations in their session. Particularly useful for multiple-thread sessions, where one "monitoring" thread has to know operation statuses from the whole session.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Only proto so far. Handler and tests will be added separately.
Was this patch authored or co-authored using generative AI tooling?
Yes.
Generated-by: Claude 4.5 Opus