You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2018/04/03 08:15:24 UTC
[GitHub] activemq-artemis pull request #1986: ARTEMIS-1653 Allow database tables to b...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/1986
ARTEMIS-1653 Allow database tables to be created externally
The previous commit about this feature wasn't using the right ResultSet
to count the rows in a table.
Additional checks have been added to:
- verify that the first SQL statement is a CREATE TABLE
- log if the existing rows are less than the expected ones
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-1653
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1986.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 #1986
----
commit 66c442dd6a4eae02f7ac0d84eb0bffd6df9a01b3
Author: Francesco Nigro <ni...@...>
Date: 2018-04-03T08:11:04Z
ARTEMIS-1653 Allow database tables to be created externally
The previous commit about this feature wasn't using the right ResultSet
to count the rows in a table.
Additional checks have been added to:
- verify that the first SQL statement is a CREATE TABLE
- log if the existing rows are less than the expected ones
----
---
[GitHub] activemq-artemis issue #1986: ARTEMIS-1653 Allow database tables to be creat...
Posted by nlippke <gi...@git.apache.org>.
Github user nlippke commented on the issue:
https://github.com/apache/activemq-artemis/pull/1986
@franz1981 :+1:
---
[GitHub] activemq-artemis issue #1986: ARTEMIS-1653 Allow database tables to be creat...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1986
@nlippke
> Statement-checks are all uppercase. Implementors using this method need to be aware of it
Thanks for the note :+1: I have added a `String::toUpperCase` while performing the checks as well.
Are not perfect/safe enough to be used as proper checks (throwing anything), but at least could help to spot regressions on tests due to some changes/configurations.
---
[GitHub] activemq-artemis issue #1986: ARTEMIS-1653 Allow database tables to be creat...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1986
@clebertsuconic I haven't reverted tha change made by https://github.com/apache/activemq-artemis/commit/ef74221684e8bf7f5ad0992f2c4ef81aa2ff986e but isn't necessary anymore, because with this fix it works without the change too.
@nlippke how it seems now? I have added a couple of checks to simplify spotting weird initialisation errors on tests too.
---
[GitHub] activemq-artemis pull request #1986: ARTEMIS-1653 Allow database tables to b...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1986#discussion_r178806417
--- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
@@ -191,9 +191,18 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
} else {
try (Statement statement = connection.createStatement();
ResultSet cntRs = statement.executeQuery(sqlProvider.getCountJournalRecordsSQL())) {
- if (rs.next() && rs.getInt(1) > 0) {
+ int rows;
+ if (cntRs.next() && (rows = cntRs.getInt(1)) > 0) {
logger.tracef("Table %s did exist but is not empty. Skipping initialization.", tableName);
+ if (logger.isDebugEnabled()) {
+ final long expectedRows = Stream.of(sqls).map(String::toUpperCase).filter(sql -> sql.contains("INSERT INTO")).count();
--- End diff --
Are you sure you meant this INSERT to only happen if debug?
---
[GitHub] activemq-artemis pull request #1986: ARTEMIS-1653 Allow database tables to b...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1986
---
[GitHub] activemq-artemis pull request #1986: ARTEMIS-1653 Allow database tables to b...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1986#discussion_r179441774
--- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
@@ -191,9 +191,18 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
} else {
try (Statement statement = connection.createStatement();
ResultSet cntRs = statement.executeQuery(sqlProvider.getCountJournalRecordsSQL())) {
- if (rs.next() && rs.getInt(1) > 0) {
+ int rows;
+ if (cntRs.next() && (rows = cntRs.getInt(1)) > 0) {
logger.tracef("Table %s did exist but is not empty. Skipping initialization.", tableName);
+ if (logger.isDebugEnabled()) {
+ final long expectedRows = Stream.of(sqls).map(String::toUpperCase).filter(sql -> sql.contains("INSERT INTO")).count();
--- End diff --
@clebertsuconic PLease do not merge it yet...because on DB2 it is creating some problems
---
[GitHub] activemq-artemis pull request #1986: ARTEMIS-1653 Allow database tables to b...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1986#discussion_r179124397
--- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
@@ -191,9 +191,18 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
} else {
try (Statement statement = connection.createStatement();
ResultSet cntRs = statement.executeQuery(sqlProvider.getCountJournalRecordsSQL())) {
- if (rs.next() && rs.getInt(1) > 0) {
+ int rows;
+ if (cntRs.next() && (rows = cntRs.getInt(1)) > 0) {
logger.tracef("Table %s did exist but is not empty. Skipping initialization.", tableName);
+ if (logger.isDebugEnabled()) {
+ final long expectedRows = Stream.of(sqls).map(String::toUpperCase).filter(sql -> sql.contains("INSERT INTO")).count();
--- End diff --
Nope, I meant that the check should be performed only with debug log level to help spotting weird behavious.
It is not the most robust check ever, but it is pretty helpfull for HA (for example).
---