-
Notifications
You must be signed in to change notification settings - Fork 356
Batch non-root inserts across aggregates #1211
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
Conversation
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 is exciting work. I left a few comments as starting points. The overall design makes sense, we should revisit the coupling between AggregateChange
and its batched variant and we should strive for keeping the API changes compatible.
...bc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateChangeExecutionContext.java
Outdated
Show resolved
Hide resolved
spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java
Outdated
Show resolved
Hide resolved
spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java
Outdated
Show resolved
Hide resolved
spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java
Outdated
Show resolved
Hide resolved
spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/data/relational/core/conversion/MutableAggregateChange.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/data/relational/core/conversion/MutableAggregateChange.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/data/relational/core/conversion/MutableAggregateChange.java
Outdated
Show resolved
Hide resolved
...n/java/org/springframework/data/relational/core/conversion/RelationalEntityDeleteWriter.java
Outdated
Show resolved
Hide resolved
private void addDelete(DbAction.Delete<?> action) { | ||
|
||
PersistentPropertyPath<RelationalPersistentProperty> propertyPath = action.getPropertyPath(); | ||
deleteActions.merge(propertyPath, new ArrayList<>(singletonList(action)), (actions, defaultValue) -> { |
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.
Nit: Duplicate nested collection creation is costly.
… of SaveMergedAggregateChange.
+ Update #populateIdsIfNecessary return type from T to List<T>
…for batching root inserts as well.
e80798f
to
8ab97b9
Compare
MyBatis related tests fail. Previous builds didn't report those since they skipped the tests. 🤔 |
…anges into one. Remove behavior from WritingContext for creating InsertBatch in favor of SaveMergedAggregateChange. Update all save paths to use SaveMergedAggregateChange. + Update #populateIdsIfNecessary return type from T to List<T> Pull out an abstract BatchWithValue class from InsertBatch to use it for batching root inserts as well. Rename InsertBatch to BatchInsert Rename AggregateChangeWithRoot to RootAggregateChange. Original pull request #1211
That's merged. Thanks for the great work! |
This is an extension of #1159 that further batches non-root insert actions across multiple aggregates via
CrudRepository#saveAll
.The hope is that these changes pave the way for follow-on work to further batch:
#saveAll
#saveAll
#deleteAll(Iterable<? extends T> entities)
#deleteAll(Iterable<? extends T> entities)
.Related to #537