dongjoon-hyun commented on PR #57834:
URL: https://github.com/apache/spark/pull/57834#issuecomment-5247317152
Thanks for the PR! I reviewed the changes and verified the wiring on both
clients. Overall the design is solid and consistent between Scala and Python. A
few findings below.
### What I verified
- **Scala**: the channel-level `MetadataHeaderClientInterceptor` filters out
a user-provided `spark-connect-operation-id`, the non-reattachable path
attaches it via a per-call interceptor in `CustomSparkConnectBlockingStub`, and
the reattachable path covers `ExecutePlan` / `ReattachExecute` /
`ReleaseExecute` through the raw stubs in
`ExecutePlanResponseReattachableIterator`. No duplicate-header risk thanks to
the channel-level filtering.
- **Python**: all 15 `self._builder.metadata()` call sites on master
(artifact manager, analyze, config, interrupts, release_session, get_status,
fetch_error_details, clone, and the `unary_unary` cleanup path in
`plan.py#__del__`) are switched to the filtered variant — none missed.
- The Python client always sets a UUID in
`_execute_plan_request_with_metadata()`, and the Scala side guards with
`.filter(_.nonEmpty)`, so an empty header value cannot be sent.
- No public API signature changes, so no MiMa impact. All imports needed by
the new Scala test already exist in the suite.
### Issues
**1. CI failure — ruff format (must fix)**
The "Linters, licenses, and dependencies" job is failing on the new test in
`test_client.py`: the wrapped `assertEqual` fits within 100 chars, so ruff
wants it collapsed into one line:
```python
self.assertEqual(client._artifact_manager._metadata, [("authorization",
"token-1")])
```
**2. Metadata freshness regression on Python retries (recommend fixing)**
In `_execute` and `_execute_and_fetch_as_iterator`, the non-reattachable
path previously called `self._builder.metadata()` on **every retry attempt**;
this PR computes `metadata` once before the retry loop. A custom
`ChannelBuilder` that mints fresh auth tokens per `metadata()` call (exactly
the per-call freshness that the new
`test_channel_builder_metadata_is_filtered_per_call` asserts) would now reuse a
stale token across retries. Moving the
`self._execute_plan_metadata(req.operation_id)` call inside the retry loop
restores the old behavior. (The reattachable path already took a single
snapshot before this PR, so it is unchanged.)
### Minor observations (non-blocking)
- `FetchErrorDetails` does not carry the operation-id header on either
client. Since it belongs to the same operation, attaching it there too could
help gateway correlation — fine to leave out of scope.
- Test coverage: Scala covers both reattachable and non-reattachable paths,
but Python only tests the non-reattachable one; the Python reattachable
iterator's header propagation is untested.
- The init-time warning check consumes one extra `self._builder.metadata()`
call. The new test pins this behavior (token-1 goes to the artifact manager),
so it looks intentional — just noting it for builders whose `metadata()` has
side effects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
