fix: reuse DynamoEnvironmentWrapper in DynamoIdentityWrapper#6813
Open
gagantrivedi wants to merge 4 commits intomainfrom
Open
fix: reuse DynamoEnvironmentWrapper in DynamoIdentityWrapper#6813gagantrivedi wants to merge 4 commits intomainfrom
gagantrivedi wants to merge 4 commits intomainfrom
Conversation
get_segment_ids was creating a new DynamoEnvironmentWrapper on every call, each triggering a fresh boto3.resource() session. This caused excessive botocore parsing overhead and memory churn. Add constructor injection so DynamoIdentityWrapper accepts an optional DynamoEnvironmentWrapper, defaulting to creating one at init time. Update call sites to pass existing wrapper instances where available.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
for more information, see https://pre-commit.ci
Contributor
Docker builds report
|
Cast identity_uuid to str to satisfy mypy strict mode, since identity_document dict values are typed as DocumentValue (union type).
Revert import-time DI in EdgeIdentity — class-level DynamoIdentityWrapper already creates one internal DynamoEnvironmentWrapper via the default, which is sufficient since it's a singleton. Update migrator test assertion to expect environment_wrapper kwarg.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6813 +/- ##
=======================================
Coverage 98.30% 98.30%
=======================================
Files 1331 1331
Lines 49332 49346 +14
=======================================
+ Hits 48494 48508 +14
Misses 838 838 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
DynamoIdentityWrapper.get_segment_ids()was creating a newDynamoEnvironmentWrapper()on every call. Each instantiation triggers a freshboto3.resource("dynamodb", ...)session with its own HTTP connection pool and botocore parser chain, causing excessive memory churn (~145MB of botocore parsing overhead per profiling run).This PR:
DynamoIdentityWrapper— it now accepts an optionalenvironment_wrapperparameter, defaulting to creating one at init time rather than perget_segment_idscall.EdgeIdentity,environments/tasks.py,migrator.py) to pass existingDynamoEnvironmentWrapperinstances instead of letting each wrapper create its own.How did you test this code?
test_get_segment_ids__called_multiple_times__reuses_environment_wrapper) that callsget_segment_idsthree times and assertsDynamoEnvironmentWrapperis instantiated only once. This test fails before the fix (call count = 3) and passes after (call count = 1).