[#3269] fix(jdbc-mysql): Solve DatabaseMetaData#getSchema return null cause list tables error on some jdbc 8.x driver #3294
Conversation
|
@kalencaya |
yes |
yuqi1129
added
branch-0.5
need backport
|
Is this worked for jdbc-doris？ |
yes, works on jdbc-doris |
I have no idea about UT/IT verify my works.now jdbc-common、jdbc-mysql、jdbc-postgresql、jdbc-doris all have releated IT testing affected code. but issue is caused by a mysql 8.x driver which need some suggest @yuqi1129 local development environment build and compileDistribution work well, jdbc-mysql and jdbc-doris using
|
You can refer to |
|
@yuqi1129 I have updated releated IT, could you help review this? |
| @@ -1351,6 +1355,11 @@ void testMySQLSchemaNameCaseSensitive() { | |||
| .stream() | |||
| .anyMatch(n -> n.equals(tableName))); | |||
| } | |||
|
|
|||
| for (String schema : schemas) { | |||
There was a problem hiding this comment.
Can you explain why we need these modifications in the test after your changes?
There was a problem hiding this comment.
After I had added a IT which CatalogMysqlDriverIT extends CatalogMysqlIT, There is a strange appearance on CatalogMysqlIT#testNameSpec, testSchemaComment, testMySQLSchemaNameCaseSensitive test. All of them run failed caused by database exists exception, because they create a new database for themself test purpose.
Strange thing is that why added CatalogMysqlDriverIT test class affect CatalogMysqlIT, I think they all will run on seperated mysql container for CatalogMysqlIT#startup() method:
@BeforeAll
public void startup() throws IOException, SQLException {
if (!ITUtils.EMBEDDED_TEST_MODE.equals(testMode)) {
String gravitinoHome = System.getenv("GRAVITINO_HOME");
Path tmpPath = Paths.get(gravitinoHome, "/catalogs/jdbc-mysql/libs");
JdbcDriverDownloader.downloadJdbcDriver(mysqlDriverDownloadUrl, tmpPath.toString());
}
TEST_DB_NAME = TestDatabaseName.MYSQL_CATALOG_MYSQL_IT;
if (mysqlImageName.equals("mysql:5.7")) {
containerSuite.startMySQLVersion5Container(TestDatabaseName.MYSQL_CATALOG_MYSQL_IT);
MYSQL_CONTAINER = containerSuite.getMySQLVersion5Container();
} else {
containerSuite.startMySQLContainer(TEST_DB_NAME);
MYSQL_CONTAINER = containerSuite.getMySQLContainer();
}
...
}
I tried some ways and didn't find the indeed reason, so I add some clear clean schema behavior on tests to avoid tests run failed.
There was a problem hiding this comment.
I checked containerSuite.startMySQLContainer(TEST_DB_NAME);. I have a misunderstood that it didn't start a new container always, so new added CatalogMysqlDriverIT and extends CatalogMysqlIT IT using the same mysql container.
CatalogMysqlIT#testNameSpec, testSchemaComment, testMySQLSchemaNameCaseSensitive test create schema and don't clean them cause database exists exception
There was a problem hiding this comment.
Dropping the database after the tests won't guarantee success if they are run in parallel. You might be able to confirm that the newly added tests CatalogMysqlDriverIT and CatalogMysqlIT always run in serialization.
There was a problem hiding this comment.
Dropping the database after the tests won't guarantee success if they are run in parallel. You might be able to confirm that the newly added tests CatalogMysqlDriverIT and CatalogMysqlIT always run in serialization.
not IT run parallel but ITs use the same mysql container.
@Tag("gravitino-docker-it")
@TestInstance(Lifecycle.PER_CLASS)
public class CatalogMysqlIT extends AbstractIT {
private static final ContainerSuite containerSuite = ContainerSuite.getInstance();
...
private MySQLContainer MYSQL_CONTAINER;
...
@BeforeAll
public void startup() throws IOException, SQLException {
...
if (mysqlImageName.equals("mysql:5.7")) {
containerSuite.startMySQLVersion5Container(TestDatabaseName.MYSQL_CATALOG_MYSQL_IT);
MYSQL_CONTAINER = containerSuite.getMySQLVersion5Container();
} else {
containerSuite.startMySQLContainer(TEST_DB_NAME);
MYSQL_CONTAINER = containerSuite.getMySQLContainer();
}
...
}
...
CatalogMysqlIT and new added CatalogMysqlDriverIT use the same mysql container, CatalogMysqlVersion5IT start a new mysql 5.7 container that's why CatalogMysqlVersion5IT extends CatalogMysqlIT similarly and not affected by database exists exception
containerSuite.startMySQLContainer(TEST_DB_NAME);
MYSQL_CONTAINER = containerSuite.getMySQLContainer();
CatalogMysqlIT#stop or CatalogMysqlIT#resetSchema method can't clear every test created schemas, only clear the global schema
public class CatalogMysqlIT extends AbstractIT {
public String metalakeName = GravitinoITUtils.genRandomName("mysql_it_metalake");
public String catalogName = GravitinoITUtils.genRandomName("mysql_it_catalog");
public String schemaName = GravitinoITUtils.genRandomName("mysql_it_schema");
public String tableName = GravitinoITUtils.genRandomName("mysql_it_table");
@AfterAll
public void stop() {
clearTableAndSchema();
client.dropMetalake(NameIdentifier.of(metalakeName));
mysqlService.close();
}
@AfterEach
public void resetSchema() {
clearTableAndSchema();
createSchema();
}
private void clearTableAndSchema() {
NameIdentifier[] nameIdentifiers =
catalog.asTableCatalog().listTables(Namespace.of(metalakeName, catalogName, schemaName));
for (NameIdentifier nameIdentifier : nameIdentifiers) {
catalog.asTableCatalog().dropTable(nameIdentifier);
}
catalog.asSchemas().dropSchema(NameIdentifier.of(metalakeName, catalogName, schemaName), false);
}
There was a problem hiding this comment.
not IT run parallel but ITs use the same mysql container.
I know that they use a shared container, so if they run serialization, dropping the database after each test case should work. If they run parallel, I believe it won't solve the problem completely.
Thread1. Thread2
create db1. create db1
drop db1 drop db1
There was a problem hiding this comment.
I think we should try to clear schemas created by every test or start a seperate mysql container for every IT.
to clear created schemas when tests finish, add clear behavior on every test last or the
@AfterEachmethod. But@AfterEachmethod should be aware of what schemas to be cleared, clear behavior added on every test last may be better
Yes, this should be reasonable in any case.
There was a problem hiding this comment.
not IT run parallel but ITs use the same mysql container.
I know that they use a shared container, so if they run serialization, dropping the database after each test case should work. If they run parallel, I believe it won't solve the problem completely.
Thread1. Thread2 create db1. create db1 drop db1 drop db1
I have understood what you means, now CatalogMysqlIT and CatalogMysqlDriverIT run parallel and there is probability that database exists exception appears, but serialization will not.
I try to find out how to do this
There was a problem hiding this comment.
junit5 runs serialization default, junit5 support parallel runs and should not be enabled on gravitino.
so clear schemas after tests finish will be ok.
what about your suggest ? @yuqi1129
There was a problem hiding this comment.
junit5 runs serialization default, junit5 support parallel runs and should not be enabled on gravitino.
so clear schemas after tests finish will be ok.
what about your suggest ? @yuqi1129
Then, the current changes will resolve the problem. I'm okay with it. Let's wait for the CI to succeed.
kalencaya
force-pushed
the
fix/mysql-driver-8
branch
2 times, most recently
from
500ca63 to
a597a2c
Compare
kalencaya
force-pushed
the
fix/mysql-driver-8
branch
from
a597a2c to
a855816
Compare
yuqi1129
changed the title
… cause list tables error on some jdbc 8.x driver (#3294) <!-- 1. Title: [#<issue>] <type>(<scope>): <subject> Examples: - "[#123] feat(operator): support xxx" - "[#233] fix: check null before access result in xxx" - "[MINOR] refactor: fix typo in variable name" - "[MINOR] docs: fix typo in README" - "[#255] test: fix flaky test NameOfTheTest" Reference: https://www.conventionalcommits.org/en/v1.0.0/ 2. If the PR is unfinished, please mark this PR as draft. --> ### What changes were proposed in this pull request? replace `DatabaseMetaData#getSchema` by `DatabaseMetaData#getCatalog` and provide a more general way to get tables through jdbc driver ### Why are the changes needed? jdbc driver `DatabaseMetaData#getSchema` method return null causes list tables failure on some jdbc mysql 8.x driver (mysql-connector-java-8.0.11). Fix: #3269 ### Does this PR introduce _any_ user-facing change? not ### How was this patch tested? (Please test your changes, and provide instructions on how to test it: 1. If you add a feature or fix a bug, add a test to cover your changes. 2. If you fix a flaky test, repeat it for many times to prove it works.) --------- Co-authored-by: wangqi <[email protected]>
|
@kalencaya |
…n null cause list tables error on some jdbc 8.x driver (apache#3294) <!-- 1. Title: [#<issue>] <type>(<scope>): <subject> Examples: - "[apache#123] feat(operator): support xxx" - "[apache#233] fix: check null before access result in xxx" - "[MINOR] refactor: fix typo in variable name" - "[MINOR] docs: fix typo in README" - "[apache#255] test: fix flaky test NameOfTheTest" Reference: https://www.conventionalcommits.org/en/v1.0.0/ 2. If the PR is unfinished, please mark this PR as draft. --> ### What changes were proposed in this pull request? replace `DatabaseMetaData#getSchema` by `DatabaseMetaData#getCatalog` and provide a more general way to get tables through jdbc driver ### Why are the changes needed? jdbc driver `DatabaseMetaData#getSchema` method return null causes list tables failure on some jdbc mysql 8.x driver (mysql-connector-java-8.0.11). Fix: apache#3269 ### Does this PR introduce _any_ user-facing change? not ### How was this patch tested? (Please test your changes, and provide instructions on how to test it: 1. If you add a feature or fix a bug, add a test to cover your changes. 2. If you fix a flaky test, repeat it for many times to prove it works.) --------- Co-authored-by: wangqi <[email protected]>
What changes were proposed in this pull request?
replace
DatabaseMetaData#getSchemabyDatabaseMetaData#getCatalogand provide a more general way to get tables through jdbc driverWhy are the changes needed?
jdbc driver
DatabaseMetaData#getSchemamethod return null causes list tables failure on some jdbc mysql 8.x driver (mysql-connector-java-8.0.11).Fix: #3269
Does this PR introduce any user-facing change?
not
How was this patch tested?
(Please test your changes, and provide instructions on how to test it: