[GitHub] activemq-artemis pull request #1822: ARTEMIS-1653 Allow database tables to b...
Github user nlippke commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1822#discussion_r178460259
--- Diff:
artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java
---
@@ -189,17 +187,27 @@ private static void createTableIfNotExists(Connection
connection,
if (sqlWarning != null) {
logger.warn(JDBCUtils.appendSQLExceptionDetails(new
StringBuilder(), sqlWarning));
}
- try (Statement statement = connection.createStatement()) {
- for (String sql : sqls) {
- statement.executeUpdate(sql);
- final SQLWarning statementSqlWarning =
statement.getWarnings();
- if (statementSqlWarning != null) {
-
logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder(),
statementSqlWarning, sql));
- }
+} else {
+ try (Statement statement = connection.createStatement();
+ ResultSet cntRs =
statement.executeQuery(sqlProvider.getCountJournalRecordsSQL())) {
+ if (rs.next() && rs.getInt(1) > 0) {
--- End diff --
@franz1981, @clebertsuconic: Indeed, the branch was broken. I've fixed it
with the following patch:
~~~
---
a/artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java
+++
b/artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java
@@ -190,8 +190,9 @@ public abstract class AbstractJDBCDriver {
} else {
try (Statement statement = connection.createStatement();
ResultSet cntRs =
statement.executeQuery(sqlProvider.getCountJournalRecordsSQL())) {
- if (rs.next() && rs.getInt(1) > 0) {
+ if (cntRs.next() && cntRs.getInt(1) > 0) {
logger.tracef("Table %s did exist but is not empty.
Skipping initialization.", tableName);
+ return;
} else {
sqls = Arrays.copyOfRange(sqls, 1, sqls.length);
}
~~~
With this patch the tests run fine and I do not get those PK violations
anymore. Please have look.
---
[GitHub] activemq-artemis pull request #1822: ARTEMIS-1653 Allow database tables to b...
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1822#discussion_r178098492
--- Diff:
artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java
---
@@ -189,17 +187,27 @@ private static void createTableIfNotExists(Connection
connection,
if (sqlWarning != null) {
logger.warn(JDBCUtils.appendSQLExceptionDetails(new
StringBuilder(), sqlWarning));
}
- try (Statement statement = connection.createStatement()) {
- for (String sql : sqls) {
- statement.executeUpdate(sql);
- final SQLWarning statementSqlWarning =
statement.getWarnings();
- if (statementSqlWarning != null) {
-
logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder(),
statementSqlWarning, sql));
- }
+} else {
+ try (Statement statement = connection.createStatement();
+ ResultSet cntRs =
statement.executeQuery(sqlProvider.getCountJournalRecordsSQL())) {
+ if (rs.next() && rs.getInt(1) > 0) {
--- End diff --
@nlippke @clebertsuconic has fixed the failed test by changing it but there
are now new ERROR logs on each test, like:
```
[main] 17:39:46,342 ERROR
[org.apache.activemq.artemis.jdbc.store.drivers.AbstractJDBCDriver]
SQL STATEMENTS:
INSERT INTO NODE_MANAGER_STORE (ID) VALUES (3)
INSERT INTO NODE_MANAGER_STORE (ID) VALUES (0)
INSERT INTO NODE_MANAGER_STORE (ID) VALUES (1)
INSERT INTO NODE_MANAGER_STORE (ID) VALUES (2)
SQL EXCEPTIONS:
SQLState: 23505 ErrorCode: 2 Message: The statement was aborted because
it would have caused a duplicate key value in a unique or primary key
constraint or unique index identified by 'SQL180329173946240' defined on
'NODE_MANAGER_STORE'.
```
I will be able to return on it from 3 April, but feel free to give it a
look, because it seems related to this PR.
---
[GitHub] activemq-artemis pull request #1822: ARTEMIS-1653 Allow database tables to b...
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1822#discussion_r177985094
--- Diff:
artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java
---
@@ -189,17 +187,27 @@ private static void createTableIfNotExists(Connection
connection,
if (sqlWarning != null) {
logger.warn(JDBCUtils.appendSQLExceptionDetails(new
StringBuilder(), sqlWarning));
}
- try (Statement statement = connection.createStatement()) {
- for (String sql : sqls) {
- statement.executeUpdate(sql);
- final SQLWarning statementSqlWarning =
statement.getWarnings();
- if (statementSqlWarning != null) {
-
logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder(),
statementSqlWarning, sql));
- }
+} else {
+ try (Statement statement = connection.createStatement();
+ ResultSet cntRs =
statement.executeQuery(sqlProvider.getCountJournalRecordsSQL())) {
+ if (rs.next() && rs.getInt(1) > 0) {
--- End diff --
@nlippke @clebertsuconic This PR is causing a failure on CI for
`NettyFailoverTest::testFailBack`, please check it to see what is happening
:+1:
---
[GitHub] activemq-artemis pull request #1822: ARTEMIS-1653 Allow database tables to b...
Github user asfgit closed the pull request at: https://github.com/apache/activemq-artemis/pull/1822 ---
[GitHub] activemq-artemis pull request #1822: ARTEMIS-1653 Allow database tables to b...
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1822#discussion_r177442175
--- Diff:
artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java
---
@@ -189,17 +187,27 @@ private static void createTableIfNotExists(Connection
connection,
if (sqlWarning != null) {
logger.warn(JDBCUtils.appendSQLExceptionDetails(new
StringBuilder(), sqlWarning));
}
- try (Statement statement = connection.createStatement()) {
- for (String sql : sqls) {
- statement.executeUpdate(sql);
- final SQLWarning statementSqlWarning =
statement.getWarnings();
- if (statementSqlWarning != null) {
-
logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder(),
statementSqlWarning, sql));
- }
+} else {
+ try (Statement statement = connection.createStatement();
+ ResultSet cntRs =
statement.executeQuery(sqlProvider.getCountJournalRecordsSQL())) {
+ if (rs.next() && rs.getInt(1) > 0) {
--- End diff --
I will merge it this.. but I'm a bit concerned with the select count taking
forever in big tables.
---
[GitHub] activemq-artemis pull request #1822: ARTEMIS-1653 Allow database tables to b...
Github user nlippke commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1822#discussion_r172812406
--- Diff:
artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java
---
@@ -109,7 +109,7 @@ public void stop() throws SQLException {
protected abstract void createSchema() throws SQLException;
protected final void createTable(String... schemaSqls) throws
SQLException {
- createTableIfNotExists(connection, sqlProvider.getTableName(),
schemaSqls);
+ createTableIfNotExists(sqlProvider.getTableName(), schemaSqls);
--- End diff --
`createTableIfNotExists` was `private static` before. I needed to change it
to non-static in order for `TestJDBCDriver` to be able to access the underlying
connection to create the empty table for the test.
---
[GitHub] activemq-artemis pull request #1822: ARTEMIS-1653 Allow database tables to b...
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1822#discussion_r172699944
--- Diff:
artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java
---
@@ -109,7 +109,7 @@ public void stop() throws SQLException {
protected abstract void createSchema() throws SQLException;
protected final void createTable(String... schemaSqls) throws
SQLException {
- createTableIfNotExists(connection, sqlProvider.getTableName(),
schemaSqls);
+ createTableIfNotExists(sqlProvider.getTableName(), schemaSqls);
--- End diff --
easier to ask than reading the code :)
Why removing this parameter here?
(sorry if I'm lazy.. I have a bunch of other PRs to check.. so it was
easier to ask).
---
[GitHub] activemq-artemis pull request #1822: ARTEMIS-1653 Allow database tables to b...
Github user nlippke commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1822#discussion_r168707901
--- Diff:
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/jdbc/JdbcLeaseLockTest.java
---
@@ -33,13 +35,31 @@
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+@RunWith(Parameterized.class)
public class JdbcLeaseLockTest extends ActiveMQTestBase {
private JdbcSharedStateManager jdbcSharedStateManager;
private DatabaseStorageConfiguration dbConf;
private SQLProvider sqlProvider;
+ @Parameterized.Parameters(name = "create_tables_prior_test")
+ public static List data() {
+ return Arrays.asList(new Object[][] {
+ {true, null},
+ {false, null}
+ });
+ }
+
+ @Parameter(0)
+ public boolean withExistingTable;
+ @Parameter(1)
+ public Object result;
--- End diff --
Your version of Junit requires to work with tuples (input, expecte result).
Newer versions allow input only. If you don't have this annotation JUnit will
complain.
---
[GitHub] activemq-artemis pull request #1822: ARTEMIS-1653 Allow database tables to b...
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1822#discussion_r168704701
--- Diff:
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/jdbc/JdbcLeaseLockTest.java
---
@@ -33,13 +35,31 @@
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+@RunWith(Parameterized.class)
public class JdbcLeaseLockTest extends ActiveMQTestBase {
private JdbcSharedStateManager jdbcSharedStateManager;
private DatabaseStorageConfiguration dbConf;
private SQLProvider sqlProvider;
+ @Parameterized.Parameters(name = "create_tables_prior_test")
+ public static List data() {
+ return Arrays.asList(new Object[][] {
+ {true, null},
+ {false, null}
+ });
+ }
+
+ @Parameter(0)
+ public boolean withExistingTable;
+ @Parameter(1)
+ public Object result;
--- End diff --
'result` isn't used, it is needed?
---
