paulcaron16k commented on PR #3783: URL: https://github.com/apache/iceberg-python/pull/3783#issuecomment-5586210023
Worth checking before this lands: it changes `S3FileSystem` from cached to per-call. Calling `_s3()` 200 times with identical properties gives 200 filesystems and 200 botocore clients on this branch, against 1 and 1 on `main`. The session is a constructor kwarg, so it joins fsspec's instance-cache key, and a fresh `AioSession` is built each call — passing the *same* session twice does cache correctly, so it's the newness, not the kwarg. The obvious remedy is to memoise the session, and I'd hold off on that: `S3V4RestSigner.__init__` creates one `requests.Session` and `__call__` uses it, so a memoised session puts one non-thread-safe `requests.Session` under every worker in PyIceberg's concurrent write path. We measured that separately — 4–6 append failures per 150 writes against a remote-signing catalog, zero with a thread-local session. This PR happens to produce 2 sessions rather than 1 in that path, which dilutes it without fixing it. So the caching cost is real but the fix for it is gated on making the signer thread-safe. Happy to raise that as its own issue with the measurements if it would help — it's independent of this PR, and this PR doesn't make it worse. -- 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]
