datastrato / gravitino

World's most powerful open data catalog for building a high-performance, geo-distributed and federated metadata lake.
https://datastrato.ai/docs/
Apache License 2.0
613 stars 193 forks source link

[#2807]: Improvement(IT): manage mysql containers in ContainerSuite #2813

Closed unknowntpo closed 1 month ago

unknowntpo commented 2 months ago

What changes were proposed in this pull request?

Manage MySQL containers (mysql:5.7, mysql:8.0) in ContainerSuite. And each test class will create a unique database named TestClass.class.getSimpleName() + TestClass.class.hashCode() for testing.

Why are the changes needed?

Because we want to share containers and manage their lifecycles in ContainerSuite.

Fix: #2807

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tested in integration tests.

unknowntpo commented 2 months ago

The MySQL container in AbstractIT should also be managed:

https://github.com/datastrato/gravitino/blob/5b5a3a4005cc6c90d0d07cf40ae794782042783d/integration-test-common/src/test/java/com/datastrato/gravitino/integration/test/util/AbstractIT.java#L170-L175

It's a bit confusing to create mysql container here, because in subclass of AbstractIT, e.g. AuditCatalogMysqlIT, it also create a mysql container and do initialization.

https://github.com/datastrato/gravitino/blob/d1b0c61c6294c095095163e1ef9f425cb2dcc800/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/AuditCatalogMysqlIT.java#L70

unknowntpo commented 2 months ago

@xunliu would you also like to review this PR ?

mchades commented 2 months ago

The MySQL container in AbstractIT should also be managed: https://github.com/datastrato/gravitino/blob/5b5a3a4005cc6c90d0d07cf40ae794782042783d/integration-test-common/src/test/java/com/datastrato/gravitino/integration/test/util/AbstractIT.java#L170-L175

It's a bit confusing to create mysql container here, because in subclass of AbstractIT, e.g. AuditCatalogMysqlIT, it also create a mysql container and do initialization.

https://github.com/datastrato/gravitino/blob/d1b0c61c6294c095095163e1ef9f425cb2dcc800/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/AuditCatalogMysqlIT.java#L70

They are for different test purposes:

I think the container can be reused

unknowntpo commented 2 months ago

@mchades I might need some help.

I've added ContainerSuite for AbstractIT, but the tests failed at here:

HadoopCatalogIT > initializationError FAILED
    java.lang.RuntimeException: Failed to operate object operation [LIST], reason [No database selected]

And I can reproduce it by manually execute these commands (copied from Github action yaml file):

./gradlew compileDistribution -x test -PjdkVersion=17
./gradlew test --rerun-tasks -PskipTests -PtestMode=deploy -PjdkVersion=17 -PskipWebITs -PjdbcBackend -PskipPythonITs

But when I use Intellij to run tests in HadoopCatalogIT, it succeed without any error.

image

unknowntpo commented 2 months ago

and BTW, some tests above has already stuck for 10 hours, and I might not have the permission to cancel it.

mchades commented 2 months ago

@mchades I might need some help.

I've added ContainerSuite for AbstractIT, but the tests failed at here:

HadoopCatalogIT > initializationError FAILED
    java.lang.RuntimeException: Failed to operate object operation [LIST], reason [No database selected]

And I can reproduce it by manually execute these commands (copied from Github action yaml file):

./gradlew compileDistribution -x test -PjdkVersion=17
./gradlew test --rerun-tasks -PskipTests -PtestMode=deploy -PjdkVersion=17 -PskipWebITs -PjdbcBackend -PskipPythonITs

But when I use Intellij to run tests in HadoopCatalogIT, it succeed without any error.

image

If it can be reproduced, we should open a new issue to track this, @yuqi1129 could you please help to confirm?

unknowntpo commented 2 months ago

@mchades @yuqi1129 now, all tests passed.

yuqi1129 commented 2 months ago

@unknowntpo Please resolve the CI error when you have free time.

yuqi1129 commented 2 months ago

Please resolve the conflict. @unknowntpo

unknowntpo commented 2 months ago

The tests failed at:

Build gravitino FAILURE reason:                                
    Execution failed for task ':web:installDeps':
        org.gradle.process.internal.ExecException: Process 'command '/home/runner/work/gravitino/gravitino/web/.gradle/pnpm/pnpm-latest/bin/pnpm'' finished with non-zero exit value 1

I think restart the CI pipeline might help.

https://github.com/datastrato/gravitino/actions/runs/8705632451/job/23876453657?pr=2813#step:6:267

yuqi1129 commented 2 months ago

@mchades Please help take a look.

unknowntpo commented 2 months ago

@yuqi1129 I've done the modifications and now all tests are passed.

unknowntpo commented 1 month ago

@mchades Would you like to do a final check on my PR ?

mchades commented 1 month ago

close and reopen for testing

unknowntpo commented 1 month ago

Thanks for reviewing @yuqi1129 and @mchades , I'll move on to #3066

mchades commented 1 month ago

Thanks for your contributions! @unknowntpo