mattyb149 commented on a change in pull request #5554:
URL: https://github.com/apache/nifi/pull/5554#discussion_r772424012
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java
##########
@@ -236,195 +246,197 @@ public void onTrigger(final ProcessContext context,
final ProcessSession session
}
int resultCount = 0;
- try (final Connection con = dbcpService.getConnection(fileToProcess ==
null ? Collections.emptyMap() : fileToProcess.getAttributes());
- final PreparedStatement st = con.prepareStatement(selectQuery)) {
- if (fetchSize != null && fetchSize > 0) {
- try {
- st.setFetchSize(fetchSize);
- } catch (SQLException se) {
- // Not all drivers support this, just log the error (at
debug level) and move on
- logger.debug("Cannot set fetch size to {} due to {}", new
Object[]{fetchSize, se.getLocalizedMessage()}, se);
+ try (final Connection con = dbcpService.getConnection(fileToProcess ==
null ? Collections.emptyMap() : fileToProcess.getAttributes())) {
+ con.setAutoCommit(context.getProperty(AUTO_COMMIT).asBoolean());
Review comment:
If autocommit is set to false, then commit() must be called explicitly
on the connection and I don't see that here. However some drivers throw an
exception if commit() is called on an autocommit connection, so I recommend
here that we save off the current value of autocommit, then call
setAutoCommit(), then at the end of processing check autocommit and call
commit() if it is false. Then we can restore the original value of autocommit
as good housekeeping.
Also for the PostgreSQL case, what is expected of the code if any error
occurs during processing? Usually for non-autocommit connections we'd call
rollback() rather than commit(). Perhaps we do something similar to the above
and check for autocommit in a `catch()` clause for the try that creates the
connection, and if false call rollback().
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java
##########
@@ -169,6 +169,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ public static final PropertyDescriptor AUTO_COMMIT = new
PropertyDescriptor.Builder()
+ .name("auto commit")
+ .displayName("Set Auto Commit")
+ .description("Enables or disables the auto commit functionality of
the DB connection. For some JDBC drivers, it is required to disable the auto
committing "
Review comment:
I'd like to see more documentation around this property to inform the
user of the behavior they can expect. For your PostgreSQL example, you could
use the doc you already have here, but in general you could describe it using
the information here:
https://docs.oracle.com/javase/tutorial/jdbc/basics/transactions.html or
whatever.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/AbstractExecuteSQL.java
##########
@@ -169,6 +169,16 @@
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.build();
+ public static final PropertyDescriptor AUTO_COMMIT = new
PropertyDescriptor.Builder()
+ .name("auto commit")
Review comment:
For consistency can we name this `esql-auto-commit`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
