-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Add Cosmos bulk execution #37033
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
base: main
Are you sure you want to change the base?
Add Cosmos bulk execution #37033
Conversation
|
Pipeline appears to have an error running EFCore.InMemory.FunctionalTests but they succeed locally. |
AndriySvyryd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add some functional tests, including ones that combine bulk execution with batching or eTags?
|
@AndriySvyryd I don't think any behavior from cosmos or the sdk changes when you enable bulk execution. I can add a tests that verify that? Basically inheriting from the tests that already exist and running those with bulk enabled |
|
I'm actually now realizing that this doesn't make a lot of sense. When using bulk, you probably want to execute the entries to save in parallel. Otherwise the only bulk that can happen is between different contexts. Will close this if you agree and see if I have time to implement that after #37027 is merged. (as it will probably have conflicts) I will have to think on how to best handle the parallelism (with manual session token management) and if we should only do in parallel with bulk or maybe increase parallelism without as well. (but probably only with bulk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds bulk execution support to the Cosmos DB provider for Entity Framework Core. The implementation enables the Cosmos DB SDK's bulk execution feature, which can improve throughput for small write operations in high-volume scenarios.
Key Changes:
- Added
BulkExecutionEnabledoption toCosmosDbContextOptionsBuilderto enable bulk execution - Modified
CosmosDatabaseWrapperto execute non-batchable writes in parallel when bulk execution is enabled - Added warning when bulk execution is used with transactional batching, since transactional batches cannot be executed in bulk mode
- Refactored test infrastructure to support skippable tests via
SkippableTestMessageBus
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.Specification.Tests/TestUtilities/Xunit/SkippableTestMessageBus.cs | New utility class to allow tests to be skipped by throwing SkipException |
| test/EFCore.Specification.Tests/TestUtilities/Xunit/ConditionalTheoryTestCase.cs | Updated to use SkippableTestMessageBus for handling skip exceptions |
| test/EFCore.Specification.Tests/TestUtilities/Xunit/ConditionalFactTestCase.cs | Updated to use SkippableTestMessageBus for handling skip exceptions |
| test/EFCore.Cosmos.Tests/Extensions/CosmosDbContextOptionsExtensionsTests.cs | Added test for BulkExecutionEnabled option |
| test/EFCore.Cosmos.FunctionalTests/EndToEndCosmosTest.cs | Refactored to support different context creation modes for bulk execution tests |
| test/EFCore.Cosmos.FunctionalTests/CosmosTransactionalBatchTest.cs | Refactored to use fixture-based testing pattern instead of NonSharedModelTestBase |
| test/EFCore.Cosmos.FunctionalTests/CosmosConcurrencyTest.cs | Added virtual CreateContext methods to support bulk execution testing |
| test/EFCore.Cosmos.FunctionalTests/CosmosBulkExecutionTest.cs | New test suite for bulk execution functionality |
| src/EFCore.Cosmos/Storage/Internal/SingletonCosmosClientWrapper.cs | Added configuration for AllowBulkExecution on CosmosClient |
| src/EFCore.Cosmos/Storage/Internal/CosmosDatabaseWrapper.cs | Implemented parallel execution of single writes when bulk execution is enabled |
| src/EFCore.Cosmos/Properties/CosmosStrings.resx | Added warning message for bulk execution with transactional batch |
| src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs | Generated code for new warning message |
| src/EFCore.Cosmos/Infrastructure/Internal/ICosmosSingletonOptions.cs | Added EnableBulkExecution property |
| src/EFCore.Cosmos/Infrastructure/Internal/CosmosSingletonOptions.cs | Implemented EnableBulkExecution property with validation |
| src/EFCore.Cosmos/Infrastructure/Internal/CosmosDbOptionExtension.cs | Added bulk execution option to extension |
| src/EFCore.Cosmos/Infrastructure/CosmosDbContextOptionsBuilder.cs | Added BulkExecutionEnabled API |
| src/EFCore.Cosmos/Diagnostics/Internal/CosmosLoggingDefinitions.cs | Added logging definition for bulk execution warning |
| src/EFCore.Cosmos/Diagnostics/Internal/CosmosLoggerExtensions.cs | Added logger extension for bulk execution warning |
| src/EFCore.Cosmos/Diagnostics/CosmosTransactionalBatchExecutedEventData.cs | Fixed typo in documentation |
| src/EFCore.Cosmos/Diagnostics/CosmosEventId.cs | Added event ID for bulk execution warning |
| src/EFCore.Cosmos/Diagnostics/BulkExecutionWithTransactionalBatchEventData.cs | New event data class for bulk execution warning |
Files not reviewed (1)
- src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs: Language not supported
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
src/EFCore.Cosmos/Diagnostics/BulkExecutionWithTransactionalBatchEventData.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Diagnostics/BulkExecutionWithTransactionalBatchEventData.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Cosmos.FunctionalTests/CosmosTransactionalBatchTest.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Diagnostics/CosmosTransactionalBatchExecutedEventData.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Diagnostics/BulkExecutionWithTransactionalBatchEventData.cs
Outdated
Show resolved
Hide resolved
| /// <remarks> | ||
| /// See <see href="https://aka.ms/efcore-docs-diagnostics">Logging, events, and diagnostics</see> for more information and examples. | ||
| /// </remarks> | ||
| public class BulkExecutionWithTransactionalBatchEventData : EventData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distinguishing feature of this class is that it contains a AutoTransactionBehavior, so rename it to AutoTransactionBehaviorEventData and adjust docs accordingly
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| throw WrapUpdateException(ex, entries.AsReadOnly()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be wrapped as it's not an exception coming from the database
| } | ||
|
|
||
| var firstEntry = entriesWithoutTriggers[0]; | ||
| var firstEntry = batchableEntries[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it safe to assume there's at least one entry in batchableEntries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand you are asking about it as it isn't intuitively clear, but we know there is at-least 1 cosmosUpdateEntries as per line 197 and we know that that update entry is not a single update as per line 224. Thus there must be at-least 1 batchableEntries
if (cosmosUpdateEntries.Count == 0 ||
_currentDbContext.Context.Database.AutoTransactionBehavior == AutoTransactionBehavior.Never ||
(cosmosUpdateEntries.Count <= 1 && _currentDbContext.Context.Database.AutoTransactionBehavior != AutoTransactionBehavior.Always))
{
return new SaveGroups ....
}
if (singleUpdateEntries.Count >= 1)
{
....
return new SaveGroups...
}| Assert.Equal(CosmosEventId.ExecutedTransactionalBatch, Fixture.ListLoggerFactory.Log[3].Id); | ||
| } | ||
|
|
||
| public class EndToEndTest(NonSharedFixture fixture) : EndToEndCosmosTest(fixture), IClassFixture<NonSharedFixture> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate the classes to other files with a prefix (Bulk, BulkNoBatch, etc.)
| using Microsoft.EntityFrameworkCore.Cosmos.Diagnostics.Internal; | ||
| using Microsoft.EntityFrameworkCore.Cosmos.Internal; | ||
|
|
||
| namespace Microsoft.EntityFrameworkCore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to Update subfolder
AndriySvyryd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to configure the warning to throw by default in
| ((IDbContextOptionsBuilderInfrastructure)optionsBuilder).AddOrUpdateExtension(coreOptionsExtension); |
See
efcore/src/EFCore.InMemory/Extensions/InMemoryDbContextOptionsExtensions.cs
Lines 156 to 157 in eeaf988
| coreOptionsExtension.WarningsConfiguration.TryWithExplicit( | |
| InMemoryEventId.TransactionIgnoredWarning, WarningBehavior.Throw)); |
Co-authored-by: Copilot <[email protected]> Co-authored-by: Andriy Svyryd <[email protected]>
Do not wrap BulkExecutionWithTransactionalBatch & dont use == true Rename BulkExecutionWithTransactionalBatchEventData
Adds BulkExecutionEnabled to CosmosDbContextOptionsBuilder
Execute non batchable document writes in parallel when bulk execution enabled
Add a warning exception when batching is enabled when using bulk execution
Implements: #36599