[GitHub] activemq-artemis issue #1997: ARTEMIS-1653 Allow database tables to be creat...
Github user nlippke commented on the issue: https://github.com/apache/activemq-artemis/pull/1997 ð ---
[GitHub] activemq-artemis pull request #1997: 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/1997#discussion_r180006216
--- Diff:
artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java
---
@@ -179,32 +179,61 @@ private void createTableIfNotExists(String tableName,
String... sqls) throws SQL
logger.tracef("Validating if table %s didn't exist before creating",
tableName);
try {
connection.setAutoCommit(false);
+ final boolean tableExists;
try (ResultSet rs = connection.getMetaData().getTables(null,
null, tableName, null)) {
-if (rs != null && !rs.next()) {
+if ((rs == null) || (rs != null && !rs.next())) {
+ tableExists = false;
if (logger.isTraceEnabled()) {
logger.tracef("Table %s did not exist, creating it with
SQL=%s", tableName, Arrays.toString(sqls));
}
- final SQLWarning sqlWarning = rs.getWarnings();
- if (sqlWarning != null) {
- logger.warn(JDBCUtils.appendSQLExceptionDetails(new
StringBuilder(), sqlWarning));
+ if (rs != null) {
+ final SQLWarning sqlWarning = rs.getWarnings();
+ if (sqlWarning != null) {
+ logger.warn(JDBCUtils.appendSQLExceptionDetails(new
StringBuilder(), sqlWarning));
+ }
}
} else {
- try (Statement statement = connection.createStatement();
- ResultSet cntRs =
statement.executeQuery(sqlProvider.getCountJournalRecordsSQL())) {
- if (rs.next() && rs.getInt(1) > 0) {
- logger.tracef("Table %s did exist but is not empty.
Skipping initialization.", tableName);
- } else {
- sqls = Arrays.copyOfRange(sqls, 1, sqls.length);
+ tableExists = true;
+}
+ }
+ if (tableExists) {
+logger.tracef("Validating if the existing table %s is
initialized or not", tableName);
+try (Statement statement = connection.createStatement();
+ ResultSet cntRs =
statement.executeQuery(sqlProvider.getCountJournalRecordsSQL())) {
+ logger.tracef("Validation of the existing table %s
initialization is started", tableName);
+ int rows;
+ if (cntRs.next() && (rows = cntRs.getInt(1)) > 0) {
+ logger.tracef("Table %s did exist but is not empty.
Skipping initialization. Found %d rows.", tableName, rows);
+ if (logger.isDebugEnabled()) {
+ final long expectedRows =
Stream.of(sqls).map(String::toUpperCase).filter(sql -> sql.contains("INSERT
INTO")).count();
+ if (rows < expectedRows) {
+logger.debug("Table " + tableName + " was expected
to contain " + expectedRows + " rows while it has " + rows + " rows.");
+ }
}
+ connection.commit();
+ return;
+ } else {
+ assert sqls[0].toUpperCase().contains("CREATE TABLE") :
"The first SQL statement must be a CREATE TABLE";
+ sqls = Arrays.copyOfRange(sqls, 1, sqls.length);
+ logger.tracef("Table %s did exist but is empty. Starting
initialization.", tableName);
+ }
+} catch (SQLException e) {
+ logger.warn(JDBCUtils.appendSQLExceptionDetails(new
StringBuilder("Can't verify the initialization of table
").append(tableName).append(" due to:"), e,
sqlProvider.getCountJournalRecordsSQL()));
+ try {
+ connection.rollback();
+ } catch (SQLException rollbackEx) {
+ logger.debug("Rollback failed while validating
initialization of a table", rollbackEx);
}
+ connection.setAutoCommit(false);
--- End diff --
Isn't `connetion` already configured as such?
---
[GitHub] activemq-artemis pull request #1997: 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/1997#discussion_r180005903
--- Diff:
artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java
---
@@ -179,32 +179,61 @@ private void createTableIfNotExists(String tableName,
String... sqls) throws SQL
logger.tracef("Validating if table %s didn't exist before creating",
tableName);
try {
connection.setAutoCommit(false);
+ final boolean tableExists;
try (ResultSet rs = connection.getMetaData().getTables(null,
null, tableName, null)) {
-if (rs != null && !rs.next()) {
+if ((rs == null) || (rs != null && !rs.next())) {
--- End diff --
`connection.getMetaData().getTables()` can return _null_?
---
[GitHub] activemq-artemis issue #1986: ARTEMIS-1653 Allow database tables to be creat...
Github user nlippke commented on the issue: https://github.com/apache/activemq-artemis/pull/1986 @franz1981 :+1: ---
[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 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 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<Object[]> 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 issue #1822: ARTEMIS-1653 Allow database tables to be creat...
Github user nlippke commented on the issue: https://github.com/apache/activemq-artemis/pull/1822 @franz1981 Done. ---
[GitHub] activemq-artemis issue #1822: Allow existing empty tables
Github user nlippke commented on the issue: https://github.com/apache/activemq-artemis/pull/1822 @michaelandrepearce, @franz1981 Done. ---
[GitHub] activemq-artemis issue #1822: Allow existing empty tables
Github user nlippke commented on the issue: https://github.com/apache/activemq-artemis/pull/1822 @franz1981 Is there a way to ask jenkins for a new build? This build error doesn't look change related and other PR builds break at the very same point. ---
[GitHub] activemq-artemis issue #1822: Allow existing empty tables
Github user nlippke commented on the issue: https://github.com/apache/activemq-artemis/pull/1822 @franz1981 Looks like I have to rebase...? ---
[GitHub] activemq-artemis pull request #1822: Allow existing empty tables
GitHub user nlippke opened a pull request: https://github.com/apache/activemq-artemis/pull/1822 Allow existing empty tables In some environments it is not allowed to create a schema by the application itself. With this change the AbstractJDBCDriver now tests if an existing table is empty and executes further statements in the same way it would if the table does not exist. You can merge this pull request into a Git repository by running: $ git pull https://github.com/nlippke/activemq-artemis allow_existing_empty_tables Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/1822.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1822 commit e3c2c59ed4029ffb7a81deb2a71c0d4135940126 Author: Niels Lippke <nlippke@...> Date: 2018-01-28T16:53:35Z Allow database tables to be created externally In some environments it is not allowed to create a schema by the application itself. With this change the AbstractJDBCDriver now tests if an existing table is empty and executes further statements in the same way as if the table does not exist. commit f75bc051fe38d9afc0058e8d16e108274eccfec4 Author: Niels Lippke <nlippke@...> Date: 2018-01-28T18:03:07Z Fixed code formatting. ---
