Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0, 2.0.0-beta3
    • Component/s: jdbc
    • Labels:
      None

      Description

      The MySQLDictionary currently supports only TYPE innodb. There is a skeleton for support of this element but there is no way to change it from the default.

      Also, according to the MySQL documentation http://dev.mysql.com/doc/refman/5.1/en/create-table.html TYPE is deprecated. The recommended (and supported) keyword is ENGINE.

      "Since MySQL 5.1.8, it produces a warning. It will be removed in a future MySQL release. You should not use TYPE in any new applications, and you should immediately begin conversion of existing applications to use ENGINE instead."

      I propose to change TYPE to ENGINE, and to provide a way for the user to specify which engine to use.

      I need some feedback on this: A property, mysql.engine, with a default of innodb (for compatibility) can be specified by the user in order to create tables using any of the supported engines. Can the property be put onto the command line? Can the property be added to persistence.xml? Can the property be added to the maven profile properties?

      1. openjpa-1530.patch
        3 kB
        Craig L Russell

        Activity

        Hide
        Craig L Russell added a comment -

        Please review this patch. It makes schema creation possible for MySQL engines other than MyISAM or innodb.

        It adds a property jdbc.mysql.Engine to the JDBC Configuration. This allows the MySQL engine type to be specified on the command line via -Dopenjpa.jdbc.mysql.Engine=ndbcluster or in persistence.xml <property name="openjpa.jdbc.mysql.Engine" value="ndbcluster".

        I didn't see other XXXDictionary implementations with special properties so I guessed this is the way to get the JDBC Configuration to hold the value and for MySQLDictionary to get the value. If there is a different pattern I should use, please let me know.

        Show
        Craig L Russell added a comment - Please review this patch. It makes schema creation possible for MySQL engines other than MyISAM or innodb. It adds a property jdbc.mysql.Engine to the JDBC Configuration. This allows the MySQL engine type to be specified on the command line via -Dopenjpa.jdbc.mysql.Engine=ndbcluster or in persistence.xml <property name="openjpa.jdbc.mysql.Engine" value="ndbcluster". I didn't see other XXXDictionary implementations with special properties so I guessed this is the way to get the JDBC Configuration to hold the value and for MySQLDictionary to get the value. If there is a different pattern I should use, please let me know.
        Hide
        Craig L Russell added a comment -

        This update should be widely available, especially for 2.0

        Show
        Craig L Russell added a comment - This update should be widely available, especially for 2.0
        Hide
        Milosz Tylenda added a comment -

        Hi Craig. The Dictionaries support their special properties just by having public variables. MySQLDictionary currently supports the "tableType" property this way. You can override it by using

        <property name="openjpa.jdbc.DBDictionary" value="mysql(TableType=ndbcluster)"/>

        or

        <property name="openjpa.jdbc.DBDictionary" value="TableType=ndbcluster"/>

        I think that the only change you want here is:

        + @Override
        public String[] getCreateTableSQL(Table table)

        { String[] sql = super.getCreateTableSQL(table); - if (!StringUtils.isEmpty(tableType)) - sql[0] = sql[0] + " TYPE = " + tableType; + if (!StringUtils.isEmpty(engine)) + sql[0] = sql[0] + " ENGINE = " + engine; return sql; }
        Show
        Milosz Tylenda added a comment - Hi Craig. The Dictionaries support their special properties just by having public variables. MySQLDictionary currently supports the "tableType" property this way. You can override it by using <property name="openjpa.jdbc.DBDictionary" value="mysql(TableType=ndbcluster)"/> or <property name="openjpa.jdbc.DBDictionary" value="TableType=ndbcluster"/> I think that the only change you want here is: + @Override public String[] getCreateTableSQL(Table table) { String[] sql = super.getCreateTableSQL(table); - if (!StringUtils.isEmpty(tableType)) - sql[0] = sql[0] + " TYPE = " + tableType; + if (!StringUtils.isEmpty(engine)) + sql[0] = sql[0] + " ENGINE = " + engine; return sql; }
        Hide
        Jeremy Bauer added a comment -

        I agree with Milosz. The property does not go beyond the scope of the dictionary so it should be localized to the MySQL dictionary. For example:

        <property name="openjpa.jdbc.DBDictionary" value="mysql(Engine=ndbcluster)"/>.

        You just need to provide a getter and setter (or public field) for "Engine" in the MySQL dictionary and the config framework will take care of setting the value. Also, if you add a new dictionary value please update the OpenJPA docs specific to the MySQL dictionary. Oh, and the print statement needs to be removed.

        Show
        Jeremy Bauer added a comment - I agree with Milosz. The property does not go beyond the scope of the dictionary so it should be localized to the MySQL dictionary. For example: <property name="openjpa.jdbc.DBDictionary" value="mysql(Engine=ndbcluster)"/>. You just need to provide a getter and setter (or public field) for "Engine" in the MySQL dictionary and the config framework will take care of setting the value. Also, if you add a new dictionary value please update the OpenJPA docs specific to the MySQL dictionary. Oh, and the print statement needs to be removed.
        Hide
        Craig L Russell added a comment -

        Do you think I need a test case for this? Seems straightforward enough and I'll be testing it pretty thoroughly myself.

        Show
        Craig L Russell added a comment - Do you think I need a test case for this? Seems straightforward enough and I'll be testing it pretty thoroughly myself.
        Hide
        Jeremy Bauer added a comment -

        IMO, no. Since it requires a special environment and will be disabled in the normal test runs anyway. But, if you'd like to add a test and put it in the excludes list that would be a bonus. It would be handy to have if it was ever needed for regression or specialized testing of that environment.

        Show
        Jeremy Bauer added a comment - IMO, no. Since it requires a special environment and will be disabled in the normal test runs anyway. But, if you'd like to add a test and put it in the excludes list that would be a bonus. It would be handy to have if it was ever needed for regression or specialized testing of that environment.
        Hide
        Michael Dick added a comment -

        I agree with Milosz & Jeremy - the engine variable should be moved to MySQLDictionary.

        If we do add a testcase I'd rather have it only run on MySQL instead of excluding it entirely (we'll forget about it if it's excluded). We'd just need to set the engine property to ndbcluster and verify that we can create a table.

        Show
        Michael Dick added a comment - I agree with Milosz & Jeremy - the engine variable should be moved to MySQLDictionary. If we do add a testcase I'd rather have it only run on MySQL instead of excluding it entirely (we'll forget about it if it's excluded). We'd just need to set the engine property to ndbcluster and verify that we can create a table.
        Hide
        Craig L Russell added a comment -

        Committed changes to 1.3.x and trunk:
        public String[] getCreateTableSQL(Table table) {
        String[] sql = super.getCreateTableSQL(table);
        if (!StringUtils.isEmpty(tableType))

        • sql[0] = sql[0] + " TYPE = " + tableType;
          + sql[0] = sql[0] + " ENGINE = " + tableType;
          return sql;

        Since tableType is already documented, there's no need to change it.

        svn commit -m "OPENJPA-1530 change TYPE to ENGINE in DDL for MySQL"
        Sending openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/MySQLDictionary.java
        Transmitting file data .
        Committed revision 920979.
        clr% svn commit -m "OPENJPA-1530 change TYPE to ENGINE in DDL for MySQL"
        Sending openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/MySQLDictionary.java
        Transmitting file data .
        Committed revision 920977.

        Show
        Craig L Russell added a comment - Committed changes to 1.3.x and trunk: public String[] getCreateTableSQL(Table table) { String[] sql = super.getCreateTableSQL(table); if (!StringUtils.isEmpty(tableType)) sql [0] = sql [0] + " TYPE = " + tableType; + sql [0] = sql [0] + " ENGINE = " + tableType; return sql; Since tableType is already documented, there's no need to change it. svn commit -m " OPENJPA-1530 change TYPE to ENGINE in DDL for MySQL" Sending openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/MySQLDictionary.java Transmitting file data . Committed revision 920979. clr% svn commit -m " OPENJPA-1530 change TYPE to ENGINE in DDL for MySQL" Sending openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/MySQLDictionary.java Transmitting file data . Committed revision 920977.
        Hide
        Craig L Russell added a comment -

        Thanks for the assistance resolving this.

        Show
        Craig L Russell added a comment - Thanks for the assistance resolving this.
        Hide
        ASF subversion and git services added a comment -

        Commit 1445923 from rpalache
        [ https://svn.apache.org/r1445923 ]

        OPENJPA-1530 change TYPE to ENGINE in DDL for MySQL

        Show
        ASF subversion and git services added a comment - Commit 1445923 from rpalache [ https://svn.apache.org/r1445923 ] OPENJPA-1530 change TYPE to ENGINE in DDL for MySQL

          People

          • Assignee:
            Craig L Russell
            Reporter:
            Craig L Russell
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development