Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cleanup Glue databases left eventually behind by other test runs #12102

Merged
merged 1 commit into from
Apr 27, 2022
Merged

Cleanup Glue databases left eventually behind by other test runs #12102

merged 1 commit into from
Apr 27, 2022

Conversation

homar
Copy link
Member

@homar homar commented Apr 22, 2022

Co-authored-by: Marius Grama findinpath@gmail.com

Description

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Apr 22, 2022
findepi
findepi reviewed Apr 22, 2022
View reviewed changes
...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java Outdated
import static org.apache.hadoop.hive.metastore.TableType.EXTERNAL_TABLE;
import static org.apache.hadoop.hive.metastore.TableType.VIRTUAL_VIEW;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

public class TestDeltaLakeGlueMetastore
{
private static final Logger log = Logger.get(TestDeltaLakeGlueMetastore.class);

private static final String TEST_DATABASE_NAME_PREFIX = "test_delta_glue";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static final String TEST_DATABASE_NAME_PREFIX = "test_delta_glue";
private static final String TEST_DATABASE_NAME_PREFIX = "test_";

cc @alexjo2144

@findepi
Copy link
Member

findepi commented Apr 22, 2022

Does this relate to @alexjo2144 's #11903 ?

@alexjo2144
Copy link
Member

If this approach is more general we can close mine

@homar homar force-pushed the homar/remove_reduntant_tests_for_delta_lake branch from dee3eff to 38f578b Compare April 25, 2022 08:39
findinpath
findinpath reviewed Apr 25, 2022
View reviewed changes
...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java Outdated
import static org.apache.hadoop.hive.metastore.TableType.EXTERNAL_TABLE;
import static org.apache.hadoop.hive.metastore.TableType.VIRTUAL_VIEW;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

public class TestDeltaLakeGlueMetastore
{
private static final Logger log = Logger.get(TestDeltaLakeGlueMetastore.class);

private static final String TEST_DATABASE_NAME_PREFIX = "test_";
Copy link
Contributor

Choose a reason for hiding this comment

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

By using this prefix will have the outcome that this test will delete glue databases belonging to other connectors as well (e.g. : iceberg).
To avoid doing the same logic for Iceberg too, can we place this logic on a shared location? Naive idea - a new module under testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@findepi WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

this is fine, let's just not introduce a new module for this

alexjo2144
alexjo2144 reviewed Apr 25, 2022
View reviewed changes
...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java Outdated
@@ -315,4 +332,38 @@ private static String randomName()
{
return UUID.randomUUID().toString().toLowerCase(ENGLISH).replace("-", "");
}

private void cleanupOrphanedDatabases()
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this in a separate test Class? It could be nice to run this class as a suite without needing to go through and clean all orphan databases

Copy link
Member Author

Choose a reason for hiding this comment

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

@alexjo2144 please tell me if this is what you wanted

Copy link
Member

Choose a reason for hiding this comment

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

It is, thanks

@homar homar force-pushed the homar/remove_reduntant_tests_for_delta_lake branch 2 times, most recently from 1044d95 to 6e9fd8a Compare April 26, 2022 09:03
findinpath
findinpath reviewed Apr 26, 2022
View reviewed changes
...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java Outdated
@@ -161,7 +161,6 @@ public void tearDown()
deleteRecursively(tempDir.toPath(), ALLOW_INSECURE);
}
});

Copy link
Contributor

Choose a reason for hiding this comment

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

not related.

@homar homar force-pushed the homar/remove_reduntant_tests_for_delta_lake branch 2 times, most recently from 515d4e0 to 270e9f2 Compare April 26, 2022 10:48
@alexjo2144
Copy link
Member

Putting it in a different class does mean you need to add it to:
https://github.com/trinodb/trino/blob/master/plugin/trino-delta-lake/pom.xml#L409
and
https://github.com/trinodb/trino/blob/master/plugin/trino-delta-lake/pom.xml#L441

@homar @findinpath
Cleanup Glue databases left eventually behind by other test runs
d21bb5a 
Co-authored-by: Marius Grama <findinpath@gmail.com>
@homar homar force-pushed the homar/remove_reduntant_tests_for_delta_lake branch from 270e9f2 to d21bb5a Compare April 26, 2022 14:49
@findepi findepi mentioned this pull request Apr 27, 2022
Closed
@findepi
Copy link
Member

findepi commented Apr 27, 2022

Test PR: #12154
(cc @ilfrin @nineinchnick for potentially streamlining this)

@homar
Copy link
Member Author

homar commented Apr 27, 2022

Looks like the new test passed
2022-04-27T11:38:51.7648265Z 2022-04-27T06:38:51.760-0500 INFO pool-3-thread-1 io.trino.testng.services.ProgressLoggingListener [BEFORE CLASS] io.trino.plugin.deltalake.metastore.glue.TestDeltaLakeCleanUpGlueMetastore 2022-04-27T11:38:51.7649688Z 2022-04-27T06:38:51.761-0500 INFO pool-3-thread-1 io.trino.testng.services.ProgressLoggingListener [TEST START] io.trino.plugin.deltalake.metastore.glue.TestDeltaLakeCleanUpGlueMetastore.cleanupOrphanedDatabases 2022-04-27T11:38:52.5043389Z 2022-04-27T06:38:52.503-0500 INFO pool-3-thread-1 io.trino.testng.services.ProgressLoggingListener [TEST SUCCESS] io.trino.plugin.deltalake.metastore.glue.TestDeltaLakeCleanUpGlueMetastore.cleanupOrphanedDatabases; (took: 0.7 seconds) 2022-04-27T11:38:52.8543709Z 2022-04-27T06:38:52.504-0500 INFO pool-3-thread-1 io.trino.testng.services.ProgressLoggingListener [AFTER CLASS] io.trino.plugin.deltalake.metastore.glue.TestDeltaLakeCleanUpGlueMetastore

@findepi
Copy link
Member

findepi commented Apr 27, 2022

CI failed: #12147

@findepi findepi merged commit 2ccf3ad into trinodb:master Apr 27, 2022
@github-actions github-actions bot added this to the 379 milestone Apr 27, 2022
@mosabua mosabua mentioned this pull request Apr 27, 2022
Merged
@ebyhr ebyhr mentioned this pull request May 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findinpath findinpath findinpath left review comments

@alexjo2144 alexjo2144 alexjo2144 approved these changes

@findepi findepi findepi approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
379
Development

Successfully merging this pull request may close these issues.
4 participants
@homar @findepi @alexjo2144 @findinpath