Skip to content

Conversation

@achrinza
Copy link
Member

@achrinza achrinza commented Jul 29, 2021

Blocked by #634
Replaces #626

Signed-off by: Sergey Nosenko [email protected]
Signed-off-by: Rifa Achrinza [email protected]

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

@achrinza achrinza force-pushed the feat/5.x-transaction-support branch from af8f99e to 5150a52 Compare July 29, 2021 12:20
@achrinza achrinza mentioned this pull request Jul 29, 2021
5 tasks
@marioestradarosa
Copy link

@darknos thanks for the PR and sorry for the inconvenience. Many developers are waiting for your new feature to be published 👍 .

@darknos
Copy link
Contributor

darknos commented Aug 6, 2021

any updates?

@achrinza achrinza force-pushed the feat/5.x-transaction-support branch from 5150a52 to 76ad701 Compare August 12, 2021 14:33
Signed-off by: Sergey Nosenko <[email protected]>
Signed-off-by: Rifa Achrinza <[email protected]>
@achrinza achrinza force-pushed the feat/5.x-transaction-support branch from 76ad701 to 91c865b Compare August 12, 2021 14:34
@achrinza achrinza marked this pull request as ready for review August 12, 2021 14:37
Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the code but the changes look reasonable to me.
As a nitpick, I wonder if we want to add some debug statement in there, like what we did in the postgresql connector. When debugging a transaction issue long time ago for that connector, it does help to have debug turned on and have something printed out. But this could be out of scope for this PR. Thanks.

UPDATED: meant to include this link: https://github.com/loopbackio/loopback-connector-postgresql/blob/master/lib/transaction.js

@marioestradarosa
Copy link

I wonder if we want to add some debug statement in there, like what we did in the postgresql connector.

The postgreSQL connector only print debug isolation level, begin, rollback & commit and those should be printed out from here instead loopback-datasource-juggler as a centralized location.

@achrinza
Copy link
Member Author

@dhmlau I propose merging this PR first, then adding the debug statement later if needed.

@marioestradarosa
Copy link

@dhmlau I propose merging this PR first, then adding the debug statement later if needed.

I believe that the debug print outs should be placed in the loopback-datasource-juggler not on every connector. The reason why is because this debug printings are general and can be caught in the loopback-datasource-juggler . We are duplicating code and effort.

Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

Agreed to land this PR first and add the debug info as a separate PR. Thanks.

@achrinza achrinza merged commit f4d42ca into 5.x Aug 18, 2021
@achrinza achrinza deleted the feat/5.x-transaction-support branch August 18, 2021 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants