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

feat: add a DynamoDBContextBuilder to construct a DynamoDBContext #3430

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

philasmar
Copy link
Contributor

@philasmar philasmar commented Aug 7, 2024

To build the low level DynamoDB requests from both high-level programming models, the SDK requires knowledge of the table and index key structure. It generally relies on an internal call to DescribeTable. This call may invoked from synchronous public APIs, which relies on the sync-over-async antipattern for versions of .NET that only have an async HTTP client. This can lead to performance issues and deadlocks.

In October 2023 we released new initializers for users to provide the table and index structure on the client and skip the DescribeTable call.

Description

The aim of this PR is to mark the methods that reply on sync-over-async calls as [Obsolete] in order to encourage users to use the new TableBuilder for the Document model, as well as the new DynamoDBContextBuilder, which is introduced in this PR, for the object-persistence model.

For the document model, this PR obsoletes all the LoadTable and TryLoadTable calls.
For the object-persistence model, this PR adds a new DynamoDBContextBuilder which constructs a DynamoDBContext that sets DisableFetchingTableMetadata to true by default.

Motivation and Context

This change is required to mode customers away from sync-over-async calls which have lead to performance issues and deadlocks in the past.

Testing

For testing, I have added new tests that use the new DynamoDBContextBuilder to create the DynamoDBContext and I left some tests that create the DynamoDBContext using the old way.
I have ran all the tests locally and they have passed.

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

Copy link

@eddiemcs3 eddiemcs3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see unit test coverage for the different methods that build the context based upon a provided client, region, and configuration. Are these somewhere that I missed?

@philasmar
Copy link
Contributor Author

I would like to see unit test coverage for the different methods that build the context based upon a provided client, region, and configuration. Are these somewhere that I missed?

The constructed DynamoDBContext does not expose any of the underlying objects that were configured by the builder, so there's nothing for me to assert on. There is a unit test that asserts an exception is thrown when 2 methods are called together, but that is the extent of it.

namespace Amazon.DynamoDBv2.DataModel
{
/// <summary>
/// Interface for a builder that constructs a <see cref="DynamoDBContext"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add would suggest giving more context for users upgrading about how this newer approach disables the DescribeTable mechanism. And explain how the can use the ConfigureContext method to revert to pervious behavior if they need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more docs in the interface and inheritor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a line that says alternatively to attributes on the class you can register the table definition using the RegisterTableDefinition method on the context object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@philasmar philasmar requested review from eddiemcs3 and normj August 8, 2024 13:46
namespace Amazon.DynamoDBv2.DataModel
{
/// <summary>
/// Interface for a builder that constructs a <see cref="DynamoDBContext"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a line that says alternatively to attributes on the class you can register the table definition using the RegisterTableDefinition method on the context object.


namespace Amazon.DynamoDBv2.DataModel
{
/// <summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to use <inheritdoc/> instead of duplicating this large comment block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@philasmar philasmar requested a review from normj August 8, 2024 19:59
@aws aws deleted a comment Aug 8, 2024
@philasmar philasmar force-pushed the asmarp/obsolete-sync-over-async branch from cb4fabc to f34782e Compare August 9, 2024 18:37
@philasmar
Copy link
Contributor Author

I rebased on top of v4-development. I'll run a dry run and merge if it succeeds.

@philasmar philasmar force-pushed the asmarp/obsolete-sync-over-async branch from d36dd11 to 857b165 Compare August 14, 2024 13:04
@philasmar philasmar merged commit 6b631bf into v4-development Aug 14, 2024
1 check passed
@dscpinheiro dscpinheiro deleted the asmarp/obsolete-sync-over-async branch August 19, 2024 00:17
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.

4 participants