dongjoon-hyun commented on PR #57834:
URL: https://github.com/apache/spark/pull/57834#issuecomment-5218700609
Thank you for working on this. The approach looks sound and the coverage is
complete: the Scala async stub (`CustomSparkConnectStub`) only serves
`addArtifacts` so no `ExecutePlan` path is missed, both Python `ExecutePlan`
call sites are updated, and the reattachable iterators carry the header on the
initial/reattach/release RPCs in both clients. The override protection is also
genuinely verified by the tests (in the Scala test, the channel-level
interceptor adds headers after the stub-level one, so without the filter the
test would read `ignored` and fail).
A few comments:
**1. Python lint will fail on `core.py` (verified locally with `ruff format
--line-length 100`)**
In `_execute_and_fetch_as_iterator`, the shortened expression is left as
three lines:
```python
it = iter(
self._stub.ExecutePlan(req, metadata=metadata)
)
```
`dev/reformat-python` collapses this to `it =
iter(self._stub.ExecutePlan(req, metadata=metadata))`, so the linter job will
fail as-is. Could you fold it into one line?
**2. Field shadowing in `CustomSparkConnectBlockingStub.executePlan`**
The new `val stub = executePlanStub(request.getOperationId)` shadows the
class field `private val stub`, so the existing `stub.executePlan(r)` call
silently switches to the new stub with no visible change in the diff. It works,
but it is easy to misread (the declaration looks unused) and fragile for future
edits. Renaming the local (e.g. `stubWithOperationId`) and updating the call
site explicitly would make the intent clear.
**3. Minor: `@volatile` in the new Scala test**
`var operationIdHeader` is written from the gRPC server thread and read from
the test thread without synchronization; `@volatile` would remove any flakiness
risk. Also, the interceptor overwrites the var on every RPC including the async
`ReleaseExecute` — benign today since the value is identical, but capturing
headers per method would be more robust and would also let the test verify the
header on `ReattachExecute`/`ReleaseExecute` (the dummy server completes
immediately, so only the initial `ExecutePlan` header is exercised now).
**4. Optional: silent drop of the user-supplied option**
A user-provided `spark-connect-operation-id` connection option is silently
discarded in both clients. This is intended and documented in the PR
description, but a one-line warning log might save someone debugging time.
--
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]
