[GitHub] [phoenix] tkhurana commented on a diff in pull request #1570: PHOENIX-6821: Optimize batching in auto-commit mode

["tkhurana (via GitHub)" <gi...@apache.org>]

tkhurana commented on code in PR #1570:
URL: https://github.com/apache/phoenix/pull/1570#discussion_r1131685158


##########
pom.xml:
##########
@@ -98,7 +98,7 @@
     <jackson-bom.version>2.12.6.20220326</jackson-bom.version>
     <antlr.version>3.5.2</antlr.version>
     <!-- Only used for tests with HBase 2.1-2.4 -->
-    <reload4j.version>1.2.19</reload4j.version>
+    <reload4j.version>1.2.21</reload4j.version>

Review Comment:
   This doesn't seem related to this PR



##########
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java:
##########
@@ -1993,19 +1992,31 @@ public void clearBatch() throws SQLException {
     @Override
     public int[] executeBatch() throws SQLException {
         int i = 0;
+        int[] returnCodes = new int [batch.size()];
+        Arrays.fill(returnCodes, -1);
+        boolean autoCommit = connection.getAutoCommit();
+        connection.setAutoCommit(false);

Review Comment:
   we need to restore the state of auto commit setting before we return from this function 



##########
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java:
##########
@@ -1993,19 +1992,31 @@ public void clearBatch() throws SQLException {
     @Override
     public int[] executeBatch() throws SQLException {
         int i = 0;
+        int[] returnCodes = new int [batch.size()];
+        Arrays.fill(returnCodes, -1);
+        boolean autoCommit = connection.getAutoCommit();
+        connection.setAutoCommit(false);
         try {
-            int[] returnCodes = new int [batch.size()];
             for (i = 0; i < returnCodes.length; i++) {
                 PhoenixPreparedStatement statement = batch.get(i);
-                returnCodes[i] = statement.execute(true) ? Statement.SUCCESS_NO_INFO : statement.getUpdateCount();
+                statement.executeForBatch();
+                returnCodes[i] = statement.getUpdateCount();
             }
             // Flush all changes in batch if auto flush is true
             flushIfNecessary();
             // If we make it all the way through, clear the batch
             clearBatch();
             return returnCodes;
-        } catch (Throwable t) {
-            throw new BatchUpdateExecution(t,i);
+        } catch (SQLException t) {
+            throw new BatchUpdateException(returnCodes, t);
+        } finally {
+            if (autoCommit) {
+                try {
+                    connection.commit();

Review Comment:
   Because you are calling commit in finally block the `clearBatch()` call has already happened so if the commit throws an exception we would have modified the state of the batch.



-- 
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@phoenix.apache.org

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