Add transactional batch support to cosmos provider #36693
Conversation
|
@dotnet-policy-service agree |
| wrapper._commandLogger.ExecutedTransactionalBatch( | ||
| response.Diagnostics.GetClientElapsedTime(), | ||
| response.Headers.RequestCharge, | ||
| response.Headers.ActivityId, | ||
| batch.Entries, | ||
| batch.CollectionId, | ||
| batch.PartitionKeyValue); |
There was a problem hiding this comment.
Since it's essentially a single database operation, just having one log entry per batch is appropriate. You can list the ids as an array [ "1", "2", "3" ] or a single ? if sensitive logging is not enabled
…to OptimisticConcurrencyException supression
| if (exception is not DbUpdateConcurrencyException | ||
| || !Dependencies.Logger.OptimisticConcurrencyException( | ||
| entry.Context, errorEntries, (DbUpdateConcurrencyException)exception, null).IsSuppressed) | ||
| batch.Items.First().Entry.Context, batch.Items.Select(x => x.Entry).ToArray(), (DbUpdateConcurrencyException)exception, null).IsSuppressed) |
There was a problem hiding this comment.
Not sure which entries to pass where here: the whole batch failed to write, but only a few items in the batch might have caused the problem. What if the user wants to reload the items and save them again? Is that possible? Or what if the user wants to retry the whole batch, but without the errored entries? Now I am passing the whole batch to Logger.OptimisticConcurrencyException and only the errored entries to DbUpdateConcurrencyException
…hrows_if_no_container
JoasE
changed the title
|
Let me know if I should rebase my changes to be a single commit with a better description |
|
@AndriySvyryd Can I get a review on this? No rush! Just trying to make sure it isn't missed/buried. Excited to see what you think |
| /// <param name="containerId">The ID of the Cosmos container being queried.</param> | ||
| /// <param name="partitionKeyValue">The key of the Cosmos partition that the query is using.</param> | ||
| /// <param name="logSensitiveData">Indicates whether the application allows logging of sensitive data.</param> | ||
| public CosmosItemReadExecutedEventData( |
There was a problem hiding this comment.
There's no need to introduce a new DTO with the same shape as an existing one (CosmosItemCommandExecutedEventData)
| context.Customers.Add(new Customer { Id = "4", PartitionKey = "2" }); | ||
| context.CustomersWithTrigger.Add(new CustomerWithTrigger { Id = "4", PartitionKey = "2" }); | ||
|
|
||
| await Assert.ThrowsAnyAsync<InvalidOperationException>(() => context.SaveChangesAsync()); |
There was a problem hiding this comment.
Assert the exception message for all InvalidOperationException caught in this class
src/EFCore.Cosmos/Storage/Internal/CosmosTransactionalBatchWrapper.cs
Outdated
Show resolved
Hide resolved
|
|
||
| using System.Collections.Generic; | ||
| using System.Net; | ||
| using System.Runtime.InteropServices; |
There was a problem hiding this comment.
Yes, I am using CollectionsMarshal.GetValueRefOrAddDefault on a dictionary
AndriySvyryd
left a comment
AndriySvyryd
left a comment
There was a problem hiding this comment.
There are some test failures, see https://github.com/dotnet/efcore/actions/runs/17581658042/artifacts/3989256339
|
@AndriySvyryd Thanks for the review! Sorry about the failing KeyValue tests! They failed for me locally with another error, but did the same on main. I assumed it was passing in the cloud since the azure pipeline succeeded, not knowing those don't run the tests. I've reset the data in my local emulator and can run the KeyValue tests now. I still have 4 other failing tests locally (on main), but these appear to have succeeded in the github action, as I couldn't find them in the test log I downloaded. |
|
Thanks for your contribution! |
2 participants
Uh oh!
There was an error while loading. Please reload this page.