Skip to content
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.

Sign up for GitHub

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

Jump to bottom

StatelessSession insertAll in batch does not do batching #2136

Merged
merged 2 commits into from
Mar 6, 2025
Merged

StatelessSession insertAll in batch does not do batching #2136

merged 2 commits into from
Mar 6, 2025

Conversation

dreab8
Copy link
Member

@dreab8 dreab8 commented Feb 26, 2025

Fix #2108

@dreab8 dreab8 mentioned this pull request Feb 26, 2025
Closed
DavideD
DavideD requested changes Mar 3, 2025
View reviewed changes
Copy link
Member

@DavideD DavideD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!
Unfortunately, there a a couple of changes needed.

DavideD
DavideD reviewed Mar 5, 2025
View reviewed changes
Copy link
Member

@DavideD DavideD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

I left a few notes, but fixing the comments is really the only thing that's missing.

I forgot we also have insertMultiple, deleteMultiple, and updateMultiple.
Do you have time to add tests for these too? Otherwise, we can do it another time.

gavinking
gavinking approved these changes Mar 6, 2025
View reviewed changes
Copy link
Member

@gavinking gavinking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@dreab8
Copy link
Member Author

dreab8 commented Mar 6, 2025

@gavinking @DavideD thanks a lot for the review

@gavinking
Copy link
Member

Are we missing a StatelessSession.upsertAll()? Did someone (me, I guess) forget about it?

@DavideD
Copy link
Member

DavideD commented Mar 6, 2025

Are we missing a StatelessSession.upsertAll()? Did someone (me, I guess) forget about it?

I feel like this issue is never going to end :D
Let's add it as new feature in separate PR/issue

@DavideD DavideD merged commit de13a27 into hibernate:main Mar 6, 2025
19 checks passed
@DavideD
Copy link
Member

DavideD commented Mar 6, 2025

Merged, thanks

@DavideD
Copy link
Member

DavideD commented Mar 6, 2025

Actually, what's the plan for all the *All methods? Are we going to remove them or are they here to stay?
I'm asking because we don't have upsertAll but we have upsertMultiple

@gavinking
Copy link
Member

Good question, I'm not sure. But I guess I'm not keen on deprecating things unless there's a good reason to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavideD DavideD DavideD requested changes

@gavinking gavinking gavinking approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

StatelessSession insertAll in batch does not do batching
3 participants
@dreab8 @gavinking @DavideD