-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
NIFI-11898: Handle auto-commit and commit based on driver capabilities in PutDatabaseRecord #7561
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.
Thanks for addressing this issue @mattyb149! The change looks straightforward, and makes the implementation more resilient for JDBC Drivers that do not support auto commit handling. I noted a couple questions.
|
||
putToDatabase(context, session, flowFile, connection); | ||
connection.commit(); | ||
// Only commit the connection if auto-commit is false | ||
if (!connection.getAutoCommit()) { |
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.
Should this be changed to check originalAutoCommit
instead of calling connection.getAutoCommit()
again?
if (!connection.getAutoCommit()) { | |
if (!originalAutoCommit) { |
|
||
@Override | ||
public String getIdentifier() { | ||
return "dbcp"; |
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.
Recommend setting a static final variable named DBCP_SERVICE_ID
and reusing across this test class.
@Override | ||
public Connection getConnection() throws ProcessException { | ||
try { | ||
Class.forName("org.apache.derby.jdbc.EmbeddedDriver"); |
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.
Is this Class.forName()
call required?
5b18f80
to
990be8d
Compare
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.
Thanks for making the adjustments @mattyb149. The test changes cover the various code paths, and runtime behavior works as expected. +1 merging
Summary
NIFI-11898 This is the
main
version of PR #7560 , please see the other PR for details.Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation