Skip to content

/ nomulus

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

Jump to bottom

Implement transactNew() in JpaTransactionManagerImpl #316

Open
wants to merge 2 commits into
base: master
from
Open

Implement transactNew() in JpaTransactionManagerImpl #316

wants to merge 2 commits into from

Conversation

@hstonec
Copy link
Collaborator

@hstonec hstonec commented Oct 21, 2019 •
edited by mindhog

This change is Reviewable
@hstonec
Rewrite transact to delegate to transactNew
Loading status checks…
08370a8
@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Oct 21, 2019

This pull request introduces 1 alert when merging fe0dea1 into 45f7c89 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null
@hstonec
Implement transactNew
Loading status checks…
22aeb0b
@hstonec hstonec force-pushed the hstonec:implement-transactnew branch from fe0dea1 to 22aeb0b Oct 21, 2019
@hstonec hstonec requested review from CydeWeys and weiminyu Oct 21, 2019
@CydeWeys
CydeWeys reviewed Oct 22, 2019
View changes
Copy link
Member

@CydeWeys CydeWeys left a comment

What is the intent of this? Why do we need it? In what circumstances do we need nesting transactions? How does the success/failure of nested transactions affect outer transactions?

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @CydeWeys and @weiminyu)
@weiminyu
weiminyu reviewed Oct 23, 2019
View changes
Copy link
Collaborator

@weiminyu weiminyu left a comment

Agree. We should review use case first and document usage guideline.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @weiminyu)
@CydeWeys
CydeWeys reviewed Oct 23, 2019
View changes
Copy link
Member

@CydeWeys CydeWeys left a comment

And to clarify, I don't think we do need this.

Reviewable status: 0 of 2 files reviewed, all discussions resolved
@hstonec
hstonec reviewed Oct 23, 2019
View changes
Copy link
Collaborator Author

@hstonec hstonec left a comment

OK, I will take a look at the current use case and update here.

Reviewable status: 0 of 2 files reviewed, all discussions resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CydeWeys CydeWeys

@weiminyu weiminyu

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@hstonec @CydeWeys @weiminyu @googlebot
You can’t perform that action at this time.