S3 Traverser: Reusing S3Client in traverser as fix for sync orchestrator scale issue #3153
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tor scale issue In sync orchestrator, we create traversers for each path prefix which were creating their own S3Client. This led to failures with scale runs. Using sync.once to make sure that S3Client is created once and reused thereafter.
…hestrator and double counting files
shnayak-msft
commented
Aug 7, 2025
| @@ -90,7 +90,7 @@ func CreateS3Client(ctx context.Context, credInfo CredentialInfo, option Credent | |||
| } | |||
| //support custom credential provider | |||
| if credInfo.S3CredentialInfo.Provider != nil { | |||
| fmt.Println("Using custom credentials") | |||
There was a problem hiding this comment.
Is this needed? I remember we saw this in logs to understand the issue and if i make it info, we may not see it anymore as we set error. We also should not see this a lot with the fix. I can leave this as is for now.
There was a problem hiding this comment.
I think it's fine to remove. @pbanakar-microsoft any thoughts?
There was a problem hiding this comment.
I think you can leave it as is for now, to validate the fix is working fine. This should get logged only twice.
otendolkar
reviewed
Aug 8, 2025
| // This is particularly useful for sync orchestrator which creates many traversers for different path prefixes | ||
| // This allows us to avoid creating a new S3 client for each traverser, improving performance and reducing resource usage. | ||
| // This is a singleton instance, so it can be shared across multiple traversers. | ||
| // It uses sync.Once to ensure that the client is created only once, even if multiple traversers are created concurrently. |
There was a problem hiding this comment.
Do we plan to use this client manager in STE as well, or will it default to using clientFactory solution. Are there any issues with that?
…es, S2sPreserveAccessTier, S2sInvalidMetadataHandleOption, S2sSourceChangeValidation, S2sGetPropertiesInBackend
…ination folder or prefix is not found
1. Performing only indexer map operations under lock instead of the whole comparison. Since this lock blocks all sync orchestrator go routines, this isolation helps with contention 2. Replacing RW locks with R locks whereever possible 3. Cleaning up comparator logic to simplify and exit sooner
1. Added different parallelism for source and target traversal to reduce waiting for target traversals. 2. Cleaned up throttler code and added safe limits based on testing 3. Handled indexer cleanup in case of target traversal failure
…timeouts for traversals.
…s and slow directory traversals
otendolkar
force-pushed
the
users/shnayak/c2c-sync-scale-fix
branch
from
428a321 to
15b6763
Compare
shnayak-msft
force-pushed
the
users/shnayak/c2c-sync-scale-fix
branch
from
5ff8477 to
15b6763
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
S3 Traverser: Reusing S3Client in traverser as fix for sync orchestrator scale issue
Feature / Bug Fix: (Brief description of the feature or issue being addressed)
Related Links:
Issues
Team thread
Documents
[Email Subject]
Type of Change
How Has This Been Tested?
Thank you for your contribution to AzCopy!