Copilot commented on code in PR #6921:
URL: https://github.com/apache/ignite-3/pull/6921#discussion_r2509077250
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/BulkLoadBenchmark.java:
##########
@@ -120,6 +132,54 @@ public static void main(String[] args) throws
RunnerException {
new Runner(opt).run();
}
+ /**
+ * Benchmark state for {@link #jdbcInsert(JdbcState)}.
+ *
+ * <p>Holds {@link Connection} and {@link PreparedStatement}.
+ */
+ @State(Scope.Benchmark)
+ public static class JdbcState {
+ private Connection connection;
+ private PreparedStatement statement;
+
+ /**
+ * Initializes session and statement.
+ */
+ @Setup
+ public void setUp() throws SQLException {
+ String queryStr = createInsertStatement();
+
+ String clientAddrs = String.join(",",
getServerEndpoints(clusterSize));
+
+ String jdbcUrl = "jdbc:ignite:thin://" + clientAddrs;
+
+ connection = DriverManager.getConnection(jdbcUrl);
+
+ connection.setAutoCommit(false);
+
+ statement = connection.prepareStatement(queryStr);
+ }
+
+ /**
+ * Closes resources.
+ */
+ @TearDown
+ public void tearDown() throws Exception {
+ connection.close();
Review Comment:
The PreparedStatement resource is not closed in tearDown, causing a resource
leak. The statement should be closed before closing the connection. Consider
using closeAll(statement, connection) to match the pattern used in other state
classes like SqlThinState.tearDown() and KvThinState.tearDown().
```suggestion
closeAll(statement, connection);
```
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/BulkLoadBenchmark.java:
##########
@@ -120,6 +132,54 @@ public static void main(String[] args) throws
RunnerException {
new Runner(opt).run();
}
+ /**
+ * Benchmark state for {@link #jdbcInsert(JdbcState)}.
+ *
+ * <p>Holds {@link Connection} and {@link PreparedStatement}.
+ */
+ @State(Scope.Benchmark)
+ public static class JdbcState {
+ private Connection connection;
+ private PreparedStatement statement;
+
+ /**
+ * Initializes session and statement.
+ */
+ @Setup
+ public void setUp() throws SQLException {
+ String queryStr = createInsertStatement();
+
+ String clientAddrs = String.join(",",
getServerEndpoints(clusterSize));
+
+ String jdbcUrl = "jdbc:ignite:thin://" + clientAddrs;
+
+ connection = DriverManager.getConnection(jdbcUrl);
+
+ connection.setAutoCommit(false);
+
+ statement = connection.prepareStatement(queryStr);
+ }
+
+ /**
+ * Closes resources.
+ */
+ @TearDown
+ public void tearDown() throws Exception {
+ connection.close();
+ }
+
+ void upload(int count, int batch) throws SQLException {
+ for (int i = 0; i < count; i++) {
+ if (i % batch == 0) {
+ connection.commit();
+ }
+
+ statement.setInt(1, i);
+ statement.executeUpdate();
+ }
Review Comment:
The commit logic is incorrect. Currently, it commits at the beginning of
each batch (when i % batch == 0), including at i=0 before any inserts. It
should commit after each batch is complete, and there should be a final commit
after the loop to ensure the last batch is committed. Compare with
SqlThinState.upload() which properly commits the previous transaction before
starting a new one and handles the final commit.
```suggestion
statement.setInt(1, i);
statement.executeUpdate();
// Commit after each batch is complete.
if ((i + 1) % batch == 0) {
connection.commit();
}
}
// Final commit for any remaining records in the last batch.
if (count % batch != 0) {
connection.commit();
}
```
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/BulkLoadBenchmark.java:
##########
@@ -120,6 +132,54 @@ public static void main(String[] args) throws
RunnerException {
new Runner(opt).run();
}
+ /**
+ * Benchmark state for {@link #jdbcInsert(JdbcState)}.
+ *
+ * <p>Holds {@link Connection} and {@link PreparedStatement}.
+ */
+ @State(Scope.Benchmark)
+ public static class JdbcState {
+ private Connection connection;
+ private PreparedStatement statement;
+
+ /**
+ * Initializes session and statement.
+ */
+ @Setup
+ public void setUp() throws SQLException {
+ String queryStr = createInsertStatement();
+
+ String clientAddrs = String.join(",",
getServerEndpoints(clusterSize));
+
+ String jdbcUrl = "jdbc:ignite:thin://" + clientAddrs;
+
+ connection = DriverManager.getConnection(jdbcUrl);
+
+ connection.setAutoCommit(false);
+
+ statement = connection.prepareStatement(queryStr);
+ }
+
+ /**
+ * Closes resources.
+ */
+ @TearDown
+ public void tearDown() throws Exception {
+ connection.close();
+ }
+
+ void upload(int count, int batch) throws SQLException {
Review Comment:
[nitpick] The upload method has package-private visibility, while the
corresponding methods in SqlThinState and KvThinState also have package-private
visibility. This is consistent, but consider making it private since it's only
called from within the benchmark class and doesn't need to be accessible
outside the package.
```suggestion
private void upload(int count, int batch) throws SQLException {
```
--
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]
