Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] DynamoDBContext TableDescription caching spike #3002

Closed
wants to merge 2 commits into from

Conversation

acraven
Copy link
Contributor

@acraven acraven commented Jul 15, 2023

Further to call with @normj and @ashovlin earlier I've sketched out a possible solution that I believe would work for our use-case where we are seeing excessive DescribeTable calls.

The aim is to reduce calls to the DescribeTable when client credentials differ per request.

Description

Added TableDescriptionCachePrefix to DynamoDBContextConfig to allow a consumer provided prefix to be used when constructing a cache key for the TableDescription cache.

There are some unrelated static Table methods that would need to be given the prefix before this could be considered complete.

Motivation and Context

The existing behaviour uses the credentials in the Dynamo client to form part of the cache key. In a multi-tenancy environment where new credentials are used per request it results in the cache key having high cardinality leading to excessive DescribeTable invocations. By allowing the consumer to provide a value for the prefix we can lower this and reduce the calls to DescribeTable for the same physical table.

While this PR won't fix #1476 I believe it will reduce the symptoms to just the first series of calls after container startup.

Testing

I've included some as yet incomplete tests

Screenshots (if appropriate)

n/a

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

@acraven acraven changed the title DynamoDBContext TableDescription caching spike [WIP] DynamoDBContext TableDescription caching spike Jul 15, 2023
@acraven
Copy link
Contributor Author

acraven commented Jul 17, 2023

Was thinking about this over the weekend and wondering what the prefix would be. I don't think we'd ever have the need to have a different prefix so a bool would work in our use-case.

@normj
Copy link
Member

normj commented Jul 19, 2023

@acraven wanted to let you know I'm reviewing the PR and thinking through the ramifications. I'm wondering if instead of a SingleAccountMode boolean we should have an enum CacheKeyGenerationMode property. The default being AccountTable and then we could add TableName for your use case. Like I said still thinking it through but wanted to let you know the PR is getting some eyes on it.

@ashovlin
Copy link
Member

Thanks again for the start, continuing this over in #3025 as we discussed.

  • Followed Norm's advice to use an enum instead of a bool. This typically gives us more flexibility in the future if we ever need to add a new value.
  • Implemented the tests you sketched out.

@ashovlin
Copy link
Member

ashovlin commented Aug 31, 2023

We finished this off in #3025, and released it yesterday as part of AWSSDK.DynamoDBv2 version 3.7.201. Thanks again for getting this started!

@ashovlin ashovlin closed this Aug 31, 2023
@acraven
Copy link
Contributor Author

acraven commented Sep 1, 2023

Thanks for completing this @ashovlin and @normj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DynamoDb .NET threadpool starvation
3 participants