[SPARK-58451][CONNECT] Filter null extension elements returned by GetStatus plugins - #57638
[SPARK-58451][CONNECT] Filter null extension elements returned by GetStatus plugins#57638haoyangeng-db wants to merge 1 commit into
Conversation
… plugins ### What changes were proposed in this pull request? `SparkConnectGetStatusHandler` collects response extensions from every registered `GetStatusPlugin` and adds them to the response with `GetStatusResponse.Builder.addExtensions` / `OperationStatus.Builder.addExtensions`. Those protobuf builder methods reject `null`, so a plugin that returns a list containing a `null` element makes the whole `GetStatus` RPC fail with a NullPointerException that escapes the per-plugin `try`/`catch` isolation. This filters `null` elements out inside that isolation boundary, at both the request level and the operation level, so one misbehaving plugin can no longer break the RPC for everyone. ### Why are the changes needed? The handler already goes to some length to isolate plugin failures: each plugin call is wrapped in `try`/`catch NonFatal`, and a throwing plugin is logged and skipped so healthy plugins still contribute. A `null` list element defeats that, because the NullPointerException is raised later by the builder, outside the `try` block, and fails the request. Plugins are third-party extension points, so the handler should not assume their returned lists are null-free. ### Does this PR introduce _any_ user-facing change? No. With well-behaved plugins the behavior is unchanged; only the failure mode for a plugin returning a null element changes, from a failed RPC to that element being dropped. ### How was this patch tested? Added `GetStatusHandlerSuite."GetStatus filters null extension elements returned by a plugin"`, which registers a plugin returning a single-null list alongside a healthy echo plugin and asserts the null is dropped while the healthy plugin's extension survives, at both the request and operation levels. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.5)
|
LGTM, thank you @haoyangeng-db! cc @HyukjinKwon |
dongjoon-hyun
left a comment
There was a problem hiding this comment.
This code change needs a JIRA ID, @haoyangeng-db .
To @uros-b , the Apache Spark community recommends to use JIRA IDs.
If some PR needs a test coverage, it means it needs JIRA IDs definitely. 😄
Done. Thanks for the heads-up! |
|
Thank you @dongjoon-hyun! Good point about Jira ID here - thank you @haoyangeng-db for creating the ticket |
HyukjinKwon
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 0 nits.
Clean, correctly-scoped null-safety guard against misbehaving third-party plugins; well tested.
Verification
Confirmed the filter sits inside the plugin-isolation try/catch (so a null element is handled like any other plugin fault) and is applied identically to processRequestExtensions and processOperationExtensions. The new NullElementGetStatusPlugin test returns a null-containing list and verifies it is filtered.
|
Thanks for the reviews @uros-b @dongjoon-hyun @HyukjinKwon ! Could one of you help merging if this looks reasonable now? |
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Thank you, @haoyangeng-db and all.
…Status plugins ### What changes were proposed in this pull request? `SparkConnectGetStatusHandler` collects response extensions from every registered `GetStatusPlugin` and adds them to the response with `GetStatusResponse.Builder.addExtensions` / `OperationStatus.Builder.addExtensions`. Those protobuf builder methods reject `null`, so a plugin that returns a list containing a `null` element makes the whole `GetStatus` RPC fail with a NullPointerException that escapes the per-plugin `try`/`catch` isolation. This filters `null` elements out inside that isolation boundary, at both the request level and the operation level, so one misbehaving plugin can no longer break the RPC for everyone. ### Why are the changes needed? The handler already goes to some length to isolate plugin failures: each plugin call is wrapped in `try`/`catch NonFatal`, and a throwing plugin is logged and skipped so healthy plugins still contribute. A `null` list element defeats that, because the NullPointerException is raised later by the builder, outside the `try` block, and fails the request. Plugins are third-party extension points, so the handler should not assume their returned lists are null-free. ### Does this PR introduce _any_ user-facing change? No. With well-behaved plugins the behavior is unchanged; only the failure mode for a plugin returning a null element changes, from a failed RPC to that element being dropped. ### How was this patch tested? Added `GetStatusHandlerSuite."GetStatus filters null extension elements returned by a plugin"`, which registers a plugin returning a single-null list alongside a healthy echo plugin and asserts the null is dropped while the healthy plugin's extension survives, at both the request and operation levels. ### Was this patch authored or co-authored using generative AI tooling? Co-authored by Claude Code. Closes #57638 from haoyangeng-db/getstatus-plugin-null-extensions. Authored-by: haoyangeng-db <haoyan.geng@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 26b3fe6) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…Status plugins ### What changes were proposed in this pull request? `SparkConnectGetStatusHandler` collects response extensions from every registered `GetStatusPlugin` and adds them to the response with `GetStatusResponse.Builder.addExtensions` / `OperationStatus.Builder.addExtensions`. Those protobuf builder methods reject `null`, so a plugin that returns a list containing a `null` element makes the whole `GetStatus` RPC fail with a NullPointerException that escapes the per-plugin `try`/`catch` isolation. This filters `null` elements out inside that isolation boundary, at both the request level and the operation level, so one misbehaving plugin can no longer break the RPC for everyone. ### Why are the changes needed? The handler already goes to some length to isolate plugin failures: each plugin call is wrapped in `try`/`catch NonFatal`, and a throwing plugin is logged and skipped so healthy plugins still contribute. A `null` list element defeats that, because the NullPointerException is raised later by the builder, outside the `try` block, and fails the request. Plugins are third-party extension points, so the handler should not assume their returned lists are null-free. ### Does this PR introduce _any_ user-facing change? No. With well-behaved plugins the behavior is unchanged; only the failure mode for a plugin returning a null element changes, from a failed RPC to that element being dropped. ### How was this patch tested? Added `GetStatusHandlerSuite."GetStatus filters null extension elements returned by a plugin"`, which registers a plugin returning a single-null list alongside a healthy echo plugin and asserts the null is dropped while the healthy plugin's extension survives, at both the request and operation levels. ### Was this patch authored or co-authored using generative AI tooling? Co-authored by Claude Code. Closes #57638 from haoyangeng-db/getstatus-plugin-null-extensions. Authored-by: haoyangeng-db <haoyan.geng@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 26b3fe6) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
|
Merged for Apache Spark 4.3.0. |
What changes were proposed in this pull request?
SparkConnectGetStatusHandlercollects response extensions from every registeredGetStatusPluginand adds them to the response withGetStatusResponse.Builder.addExtensions/OperationStatus.Builder.addExtensions. Those protobuf builder methods rejectnull, so a plugin that returns a list containing anullelement makes the wholeGetStatusRPC fail with a NullPointerException that escapes the per-plugintry/catchisolation.This filters
nullelements out inside that isolation boundary, at both the request level and the operation level, so one misbehaving plugin can no longer break the RPC for everyone.Why are the changes needed?
The handler already goes to some length to isolate plugin failures: each plugin call is wrapped in
try/catch NonFatal, and a throwing plugin is logged and skipped so healthy plugins still contribute. Anulllist element defeats that, because the NullPointerException is raised later by the builder, outside thetryblock, and fails the request. Plugins are third-party extension points, so the handler should not assume their returned lists are null-free.Does this PR introduce any user-facing change?
No. With well-behaved plugins the behavior is unchanged; only the failure mode for a plugin returning a null element changes, from a failed RPC to that element being dropped.
How was this patch tested?
Added
GetStatusHandlerSuite."GetStatus filters null extension elements returned by a plugin", which registers a plugin returning a single-null list alongside a healthy echo plugin and asserts the null is dropped while the healthy plugin's extension survives, at both the request and operation levels.Was this patch authored or co-authored using generative AI tooling?
Co-authored by Claude Code.