Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Ensure use of InnoDB engine on MySQL #773

Merged
merged 7 commits into from
May 5, 2015
Merged

Ensure use of InnoDB engine on MySQL #773

merged 7 commits into from
May 5, 2015

Conversation

seanf
Copy link
Contributor

@seanf seanf commented Apr 13, 2015

https://bugzilla.redhat.com/show_bug.cgi?id=1207575

@alex-sl-eng
Copy link
Member

👍

@seanf
Copy link
Contributor Author

seanf commented Apr 16, 2015

@carlosmunoz I'd like your opinion on this.

for (int i = 0; i < sqls.length; i++) {
Sql sql = sqls[i];
String sqlText = sql.toSql();
String sqlUpper = sqlText.toUpperCase();
Copy link
Member

Choose a reason for hiding this comment

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

How about a .trim() just in case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

@carlosmunoz
Copy link
Member

Looks good in general terms. We are making some assumptions about the sql string, but I guess the check that you've added can raise a red flag if there are non-InnoDB tables in the midst.

@seanf
Copy link
Contributor Author

seanf commented Apr 16, 2015

@carlosmunoz note that this is targeting release, if that makes any difference.

@djansen-redhat
Copy link
Contributor

✅ Tested update and halt on !InnoDB

@seanf seanf removed the Reviewed label Apr 20, 2015
@seanf
Copy link
Contributor Author

seanf commented Apr 20, 2015

@djansen-redhat @aeng @carlosmunoz I have added a custom error message for the precondition check.

@seanf
Copy link
Contributor Author

seanf commented Apr 21, 2015

When I tested it again, I couldn't find any evidence that Liquibase was picking up the "ensure InnoDB" extension(!). I had to move it to its own jar before Liquibase would register it.

I've also added another extension which logs before executing each change, which should help a little with https://bugzilla.redhat.com/show_bug.cgi?id=1207980

@seanf seanf added On QA and removed Verified labels Apr 21, 2015
@djansen-redhat
Copy link
Contributor

Is there a bug report related to this?

@seanf
Copy link
Contributor Author

seanf commented Apr 21, 2015

Is there a bug report related to this?

https://bugzilla.redhat.com/show_bug.cgi?id=1207575

@alex-sl-eng
Copy link
Member

👍 on 6b111cc

@seanf
Copy link
Contributor Author

seanf commented Apr 22, 2015

@carlosmunoz How do you feel about the new module under zanata-server, ie zanata-liquibase?

I'm hoping we'll be able to move our changelogs and custom changes into that module too, which would make it easier to run liquibase from the command line.

@carlosmunoz
Copy link
Member

I don't mind it... although it looks small for the time being.

If we end up moving the changelogs, will liquibase be able to read them from inside a jar? or will we need two artifacts. In the later case, I would rather keep the changelogs where they currently reside.

@seanf
Copy link
Contributor Author

seanf commented Apr 22, 2015

On 2015-04-23 00:53, Carlos Munoz wrote:

I don't mind it... although it looks small for the time being.

If we end up moving the changelogs, will liquibase be able to read them
from inside a jar? or will we need two artifacts. In the later case, I
would rather keep the changelogs where they currently reside.

Yes, liquibase is supposed to be able to read changelogs from a jar.
The only question will be how many classes our custom changes depend on,
because they may have to be moved out of zanata.war into a jar.

Sean Flanigan

Principal Software Engineer
Localisation Tools Engineering
Red Hat

@djansen-redhat
Copy link
Contributor

Test:

  • Move !InnoDB tables to InnoDB on changeset is run
  • Abort on !InnoDB tables existing after changeset is run
  • Warn on !InnoDB non-Zanata tables existing after changeset is run?

@seanf
Copy link
Contributor Author

seanf commented Apr 27, 2015

@djansen-redhat: regarding your checklist:

  1. only for tables created since the last time we cleaned this up. So this doesn't apply to HTextFlow, for instance.
  2. correct (assuming they are in the same schema as Zanata's tables)
  3. no, we abort if the non-InnoDB tables are in the same schema, otherwise they are ignored.

@djansen-redhat
Copy link
Contributor

✅ Tested

@seanf
Copy link
Contributor Author

seanf commented Apr 30, 2015

@carlosmunoz are you happy with c69d8ec?

@carlosmunoz
Copy link
Member

👍

@seanf
Copy link
Contributor Author

seanf commented Apr 30, 2015

I need to double check that the new extension is indeed logging changeset IDs before executing them (unless someone beats me to it).

@seanf seanf mentioned this pull request May 4, 2015
@seanf seanf added On hold and removed On hold labels May 5, 2015
seanf added a commit that referenced this pull request May 5, 2015
Ensure use of InnoDB engine on MySQL
@seanf seanf merged commit 80d58ff into release May 5, 2015
@seanf seanf deleted the ensure-innodb branch May 5, 2015 01:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants