Create separate operation-specific configs for the object-mapper Batch and Transact operations#3376
Conversation
|
If we are putting the same 6 properties on all operation configs should those be in a base class? Or does that cause confusion when you get to other operations. |
I was on the fence about this. Given that we are unwinding an object that became over-shared, I opted to treat it like generated code and keep all the definitions separate. But I am planning to use those 6 for all of our current operations as they're used either at the table level or anytime we convert POCO <-> items. I suppose that if we did use a base class and added a new operation that didn't use one of the 6, then we can not use the base class. Any thoughts @96malhar or @philasmar ? |
| /// <summary> | ||
| /// The object persistence model API relies on an internal cache of the DynamoDB table's metadata to construct and validate | ||
| /// requests. This controls how the cache key is derived, which influences when the SDK will call | ||
| /// IAmazonDynamoDB.DescribeTable(string) internally to populate the cache. |
There was a problem hiding this comment.
nit: add doc reference to IAmazonDynamoDB.DescribeTable(string)
There was a problem hiding this comment.
DescribeTable isn't available in all targets, it's DescribeTableAync in .NET/Core/Standard.
I didn't think it was worth #if in the doc string, and the API reference generator is going to pick one anyway (i.e. we don't generate target-specific pages).
| /// </summary> | ||
| /// <remarks> | ||
| /// Setting this to true can avoid latency and thread starvation due to blocking asynchronous | ||
| /// IAmazonDynamoDB.DescribeTable(string) calls that are used to populate the SDK's cache of |
There was a problem hiding this comment.
nit: add doc reference to IAmazonDynamoDB.DescribeTable(string)
| /// <summary> | ||
| /// The object persistence model API relies on an internal cache of the DynamoDB table's metadata to construct and validate | ||
| /// requests. This controls how the cache key is derived, which influences when the SDK will call | ||
| /// IAmazonDynamoDB.DescribeTable(string) internally to populate the cache. |
There was a problem hiding this comment.
nit: add doc reference to IAmazonDynamoDB.DescribeTable(string)
| /// </summary> | ||
| /// <remarks> | ||
| /// Setting this to true can avoid latency and thread starvation due to blocking asynchronous | ||
| /// IAmazonDynamoDB.DescribeTable(string) calls that are used to populate the SDK's cache of |
There was a problem hiding this comment.
nit: add doc reference to IAmazonDynamoDB.DescribeTable(string)
| /// <param name="operationConfig">Config object which can be used to override that table used.</param> | ||
| /// <returns>Empty strongly-typed BatchGet object</returns> | ||
| BatchGet<T> CreateBatchGet<T>(DynamoDBOperationConfig operationConfig = null); | ||
| [Obsolete("Use the CreateBatchGet overload that takes BatchGetConfig instead, since DynamoDBOperationConfig contains properties that are not applicable to BatchGet.")] |
There was a problem hiding this comment.
why did you change the signature of the method?
There was a problem hiding this comment.
If we added something similar to what was there, with the new config as optional:
BatchGet<T> CreateBatchGet<T>(DynamoDBOperationConfig operationConfig = null);
BatchGet<T> CreateBatchGet<T>(BatchGetConfig batchGetConfig = null);
Then these two calls are both ambiguous:
context.CreateBatchGet<Foo>();
context.CreateBatchGet<Foo>(null);
If we didn't make the new config optional:
BatchGet<T> CreateBatchGet<T>(DynamoDBOperationConfig operationConfig = null);
BatchGet<T> CreateBatchGet<T>(BatchGetConfig batchGetConfig);
Then
context.CreateBatchGet<Foo>(); // Calling the now obsolete overload, which we don't want
context.CreateBatchGet<Foo>(null); // Still ambiguous
So for that context.CreateBatchGet<Foo>(); case, I added a non-obsolete parameterless CreateBatchGet<T>()
BatchGet<T> CreateBatchGet<T>();
BatchGet<T> CreateBatchGet<T>(DynamoDBOperationConfig operationConfig = null);
BatchGet<T> CreateBatchGet<T>(BatchGetConfig batchGetConfig);
With that I was worried context.CreateBatchGet<Foo>() would still be ambiguous between the first two, but reviewing https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/expressions#12643-better-function-member again the parameterless one will win. So I'll add back the middle call.
context.CreateBatchGet<Foo>(null); is still ambiguous, but I don't think we can do anything about that , nor do I think we we should, since that call doesn't add anything over context.CreateBatchGet<Foo>() that I can see.
There was a problem hiding this comment.
Added back existing optional DynamoDBOperationConfigs in latest commits.
| BatchWrite<T> CreateBatchWrite<T>(DynamoDBOperationConfig operationConfig = null); | ||
| [Obsolete("Use the CreateBatchWrite overload that takes BatchWriteConfig instead, since DynamoDBOperationConfig contains properties that are not applicable to BatchWrite.")] | ||
|
|
||
| BatchWrite<T> CreateBatchWrite<T>(DynamoDBOperationConfig operationConfig); |
There was a problem hiding this comment.
why did you change the signature of the method?
| /// <returns>Empty strongly-typed BatchWrite object</returns> | ||
| BatchWrite<object> CreateBatchWrite(Type valuesType, DynamoDBOperationConfig operationConfig = null); | ||
| [Obsolete("Use the CreateBatchWrite overload that takes BatchWriteConfig instead, since DynamoDBOperationConfig contains properties that are not applicable to BatchWrite.")] | ||
| BatchWrite<object> CreateBatchWrite(Type valuesType, DynamoDBOperationConfig operationConfig); |
There was a problem hiding this comment.
why did you change the signature of the method?
| /// <param name="operationConfig">Config object which can be used to override that table used.</param> | ||
| /// <returns>Empty strongly-typed TransactGet object.</returns> | ||
| TransactGet<T> CreateTransactGet<T>(DynamoDBOperationConfig operationConfig = null); | ||
| [Obsolete("Use the CreateTransactGet overload that takes TransactGetConfig instead, since DynamoDBOperationConfig contains properties that are not applicable to BatchGet.")] |
There was a problem hiding this comment.
why did you change the signature of the method?
| /// <returns>Empty strongly-typed TransactWrite object.</returns> | ||
| TransactWrite<T> CreateTransactWrite<T>(DynamoDBOperationConfig operationConfig = null); | ||
| [Obsolete("Use the CreateTransactWrite overload that takes TransactWriteConfig instead, since DynamoDBOperationConfig contains properties that are not applicable to CreateTransactWrite.")] | ||
| TransactWrite<T> CreateTransactWrite<T>(DynamoDBOperationConfig operationConfig); |
There was a problem hiding this comment.
why did you change the signature of the method?
I prefer to use a BaseClass. This is going to become a maintainability issue. We should not add any property to this base class that does not apply to all operations. Any property that is operation-specific should go on the operation-specific config object. |
Created #3380 to introduce the base class. Once that is merged I'll rebase this on top of it. |
…r Batch and Transact operations
ashovlin
force-pushed
the
shovlia/v4-ddb-separate-batch
branch
from
68ad0d1 to
2f5fb43
Compare
peterrsongg
left a comment
peterrsongg
left a comment
There was a problem hiding this comment.
Approved with a comment
| } | ||
|
|
||
| [TestMethod] | ||
| public void BatchGetConfig() |
There was a problem hiding this comment.
non-blocking. but maybe worth adding unit test coverage for ToDynamoDBOperationConfig. Don't have to see this added, but just commenting since this is a new method, and these tests mainly seem to be a reminder to add new properties to ToDynamoDbOperationConfig
There was a problem hiding this comment.
Added in 6895929.
It's testing that one of the properties on the new config object is correctly overriding the context-level config by inspecting the low-level request.
| /// Property that directs <see cref="DynamoDBContext"/> to use consistent reads. | ||
| /// If property is not set, behavior defaults to non-consistent reads. | ||
| /// </summary> | ||
| public bool? ConsistentRead { get; set; } |
There was a problem hiding this comment.
nit: Can you add a link to https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.ReadConsistency.html in the property comment. This will helps users exercise better judgment while setting this value.
…fic config overrides the context config.
… Batch and Transact operations (#3376)
… Batch and Transact operations (#3376)
5 participants
Description
Creates new operation-specific config objects for the DynamoDB object-mapper operations.
I'm splitting this into 3 PRs:
I have a longer analysis in an internal doc, but I'm planning to apply these properties to all operations because they deal with the table itself or the DynamoDB item <-> .NET object transformation:
OverrideTableNameTableNamePrefixMetadataCachingModeIsEmptyStringValueEnabledConversionDisableFetchingTableMetadataThen these are on a case-by-case basis:
BackwardQuery- only when queryingIndexName- only when querying or scanningConditionalOperator- only when querying or scanningQueryFilter- only when querying or scanningConsistentRead- when getting items (including querying or scanning)SkipVersionCheck- when writing or deleting itemsIgnoreNullValues- when writing itemsRetrieveDateTimeInUtc- when getting items (including querying or scannign)Motivation and Context
For V4 of the SDK, we're planning to split the shared
DynamoDBOperationConfigobject into operation-specific configs, since the shared object has grown overtime to include properties that are not relevant for all operations.DOTNET-7513
Testing
Ran DynamoDBv2 sln tests locally
Types of changes
Checklist
License