Skip to content

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

Closed
wants to merge 11 commits into from

Conversation

ctailor2
Copy link
Contributor

@ctailor2 ctailor2 commented Apr 1, 2022

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:

  1. root insert actions via #saveAll
  2. non-root delete actions via #saveAll
  3. non-root delete actions via #deleteAll(Iterable<? extends T> entities)
  4. root delete actions via #deleteAll(Iterable<? extends T> entities).

Related to #537

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 1, 2022
@ctailor2 ctailor2 requested review from schauder and mp911de April 1, 2022 19:40
Copy link
Member

@mp911de mp911de left a 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.

private void addDelete(DbAction.Delete<?> action) {

PersistentPropertyPath<RelationalPersistentProperty> propertyPath = action.getPropertyPath();
deleteActions.merge(propertyPath, new ArrayList<>(singletonList(action)), (actions, defaultValue) -> {
Copy link
Member

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.

@ctailor2 ctailor2 force-pushed the issue/537-batch-ops-across-aggregates branch from e80798f to 8ab97b9 Compare April 7, 2022 14:36
@ctailor2 ctailor2 requested a review from mp911de April 7, 2022 14:37
@schauder
Copy link
Contributor

MyBatis related tests fail.

https://jenkins.spring.io/blue/organizations/jenkins/spring-data-jdbc/detail/issue%2F537-batch-ops-across-aggregates/38/pipeline

Previous builds didn't report those since they skipped the tests. 🤔

schauder pushed a commit that referenced this pull request Jun 8, 2022
…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
schauder added a commit that referenced this pull request Jun 8, 2022
Original pull request #1211
schauder added a commit that referenced this pull request Jun 8, 2022
Introduces the BatchedActions abstraction to encapsulate the different ways singular actions get combined into batched actions.

Original pull request #1230
Original pull request #1229
Original pull request #1228
Original pull request #1211
@schauder
Copy link
Contributor

schauder commented Jun 8, 2022

That's merged.

Thanks for the great work!

@schauder schauder closed this Jun 8, 2022
@schauder schauder deleted the issue/537-batch-ops-across-aggregates branch June 8, 2022 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants