Support following S3 bucket region redirects when the CRT client is enabled - #10623
Support following S3 bucket region redirects when the CRT client is enabled#10623ashovlin wants to merge 3 commits into
Conversation
|
Added some fixes in c9feb20 after reviewing locally in a fresh Claude session:
|
| if new_region is None: | ||
| logger.debug( | ||
| f"S3 client configured for region {client_region} but the bucket {bucket} is not " | ||
| "in that region and the proper region could not be " | ||
| "automatically determined." | ||
| ) | ||
| return |
There was a problem hiding this comment.
Previously, if error was not redirect-related, it would return early and this debug log would never fire. Now, if the error is not redirect-related, it returns early only for self.get_redirect_region(). Since new_region = None in this case, the debug log fires anyways. We should break out redirect check in get_redirect_region() to something like _is_redirect_response() and return from redirect_from_error early.
| on_done_after_calls, | ||
| ) | ||
| ) | ||
| crt_client = self.get_crt_client(region) |
There was a problem hiding this comment.
Can we move client creation outside of the lock?
| if error is not None and self._can_redirect( | ||
| is_region_redirect, bytes_transferred | ||
| ): | ||
| # Discovering a region and serializing the retry can both | ||
| # block, and this runs on a CRT completion thread, where | ||
| # blocking stalls every other transfer sharing the event loop. | ||
| self._dispatch_redirect(redirect_and_finish, error, kwargs) | ||
| return |
There was a problem hiding this comment.
Based on the logic of _can_redirect() and CRTS3RegionRedirectPolicy.is_error_redirect_candidate(), we're not actually checking if it's a redirect error before spawning a thread. So any error that doesn't get filtered out spawns a crt-s3-region-redirect. Can we check the instance of error and filter out non S3 redirect errors before spawning?
| # request cannot be built for a region that changes while it is built. | ||
| # One lock covers every bucket because a command generally transfers | ||
| # to or from a single one. | ||
| self._region_lock = threading.Lock() |
There was a problem hiding this comment.
It looks like this lock is acquired for both region discovery (get_retry_region()) and cache lookup (locked_bucket_region()). The issue is that the cache lookup occurs during CRT request serialization, so it's possible for it to be blocked while get_retry_region has a lock for HeadObject. In the worst case, a long-running HeadObject (retries, timeouts, etc) can block CRT transfer requests that may have correctly configured buckets. I think a workaround here is to use 2 locks.
| bucket is resubmitted in the bucket's region, which makes a second | ||
| request for the same transfer. | ||
| """ | ||
| self._request_factory = request_factory |
There was a problem hiding this comment.
Can we clear this on complete()? It looks like the source of a memory leak
2 participants
Issue #, if available: CLI-9212
Description of changes: This adds support for following S3 bucket region redirects for the high-level
s3commands when the CRT transfer manager is enabled.CRTTransferCoordinatorstill coordinates the entire transfer for a single object. It now wraps the initial request and the optional retry, and exposes future-stylecancel/result/etc. that apply to the either transfer, and act on either underlying requestCRTTransferManager- rather than containing a single CRT client, now caches a map of regions to clientsS3RegionRedirectorv2so we can use them for CRT requests. I addedCRTS3RegionRedirectPolicyon top which checks some CRT-specific conditions, andget_bucket_regiontransforms the CRT error into somethingS3RegionRedirectorv2can work withLazyHeadBucketClient- our existing Python redirect logic supports callingHeadBucketif it receives an error but cannot parse the region from the error response headers. I didn't see this case in testing, but to preserve the code path, we just a lazy boto client if needed.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.