Skip to content

ARTEMIS-1653 Allow database tables to be created externally#1986

Closed
franz1981 wants to merge 1 commit into
apache:masterfrom
franz1981:ARTEMIS-1653
Closed

ARTEMIS-1653 Allow database tables to be created externally#1986
franz1981 wants to merge 1 commit into
apache:masterfrom
franz1981:ARTEMIS-1653

Conversation

@franz1981
Copy link
Copy Markdown
Contributor

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

@franz1981
Copy link
Copy Markdown
Contributor Author

@clebertsuconic I haven't reverted tha change made by ef74221 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.

Copy link
Copy Markdown
Contributor

@nlippke nlippke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Statement-checks are all uppercase. Implementors using this method need to be aware of it. Apart from that looks good.

@franz1981
Copy link
Copy Markdown
Contributor Author

@nlippke

Statement-checks are all uppercase. Implementors using this method need to be aware of it

Thanks for the note 👍 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.

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
@nlippke
Copy link
Copy Markdown
Contributor

nlippke commented Apr 3, 2018

@franz1981 👍

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();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you meant this INSERT to only happen if debug?

Copy link
Copy Markdown
Contributor Author

@franz1981 franz1981 Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I meant that the check should be performed only with debug log level to help spotting weird behaviours.
It is not the most robust check ever, but it is pretty helpfull for HA (for example).

Copy link
Copy Markdown
Contributor Author

@franz1981 franz1981 Apr 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clebertsuconic Please do not merge it yet...because on DB2 it is creating some problems.
I will close it to avoid being merged accidentally 👍

@franz1981 franz1981 closed this Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants