uros-b commented on code in PR #57778:
URL: https://github.com/apache/spark/pull/57778#discussion_r3719254931
##########
python/pyspark/sql/connect/client/reattach.py:
##########
@@ -109,7 +109,9 @@ def __init__(
# Initial iterator comes from ExecutePlan request.
# Note: This is not retried, because no error would ever be thrown
here, and GRPC will only
# throw error on first self._has_next().
- self._metadata = metadata
+ # Convert metadata to a list to ensure it remains re-iterable across
all RPCs
+ # (ReattachExecute, ReleaseExecute), so auth headers are always
present.
+ self._metadata = list(metadata)
Review Comment:
+1 with @HyukjinKwon's concern. The PR introduces an internal inconsistency:
self._metadata = list(metadata) (line 114) consumes the original metadata
iterable, but the ExecutePlan call immediately below (line 120) still passes
metadata=metadata; the now-potentially-exhausted original, rather than
metadata=self._metadata. For the real-world caller (ChannelBuilder.metadata()
returns a list), this is currently harmless, but the type annotation accepts
any Iterable[Tuple[str, str]], which includes one-shot generators. The partial
fix leaves the initial ExecutePlan call unchanged and creates a silent
correctness trap: if a custom ChannelBuilder passes a generator, the initial
RPC consumes it and the materialized self._metadata is empty, corrupting all
subsequent RPCs while appearing to be protected.
--
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]
