[GitHub] [nifi] mattyb149 commented on a change in pull request #5554: NIFI-8605 Adding a new property to enable/disable auto committing

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org