paulcaron16k commented on PR #3783: URL: https://github.com/apache/iceberg-python/pull/3783#issuecomment-5578367178
This also fixes #3896, which is a second, independent reason the `unregister` on line 237 is wrong: `_s3()` removes the signer and re-registers it on an emitter fsspec caches and every thread shares, so a request signed in that window goes out unsigned and the store answers `403 AccessDenied` — distinct from the `InvalidRequest` this PR describes, and reached without any lazily-created client being involved. Your change closes it because it drops the `unregister` entirely. I verified that rather than assuming: applied this PR and ran two tests that fail on `main` and pass with it — - `test_s3_leaves_a_signer_installed_while_reconfiguring_a_shared_client` — observes the shared emitter the instant `_s3()` unregisters - `test_the_signer_stays_installed_while_another_thread_reconfigures_s3` — the same window from another thread Both are in `tests/io/test_fsspec.py`, need no credentials or network, and check they are not passing vacuously under this PR (the signer is still visible on `fs.s3.meta.events`, since the client inherits the session's handlers). They are on #3896 if useful. Happy to raise them against this PR instead so the fix and its regression cover land together — your call, and no action needed from you either way. -- 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]
