Disallow dropping Hive schemas that contain external files #9740

jirassimok · 2021-10-22T13:36:50Z

This avoids potential data loss when a schema is created and dropped in a location that already has files.

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestCreateDropSchema.java

losipiuk

LGTM

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java

losipiuk · 2021-10-29T12:34:44Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java

+    private static final SchemaType DATABASE = SchemaType.DATABASE;
+    private static final SchemaType TABLE = SchemaType.TABLE;
+    private static final SchemaType PARTITION = SchemaType.PARTITION;


can you just static import those (actually I am not sure if you really can) :)

If I make the enum non-private, I could.

Yeah - would make more sense to me :)

Made it package-private with a comment.

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java

losipiuk · 2021-10-29T12:40:24Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java

+        deleteMetadataDirectory(DATABASE, databaseMetadataDirectory);
+        try {
+            if (!metadataFileSystem.delete(getSchemaPath(DATABASE, databaseMetadataDirectory), false)) {
+                throw new TrinoException(HIVE_METASTORE_ERROR, "Failed to delete database schema file");


nit: put path in exception message

losipiuk

LGTM. A few nits. To be merged after.

jirassimok · 2021-10-29T17:03:35Z

This shouldn't be merged yet, pending offline discussion.

findepi · 2021-10-30T20:53:08Z

Per offline discussion, we should unregister such schema, and leave the files on the storage intact.
Rationale for this is that a schema could have been created for an existing location (CREATE SCHEMA x WITH (location='existing/locaction/with/some/existing/data')). This PR would prevent such schema from being deleted.

jirassimok · 2021-11-05T03:14:10Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

@@ -783,7 +783,27 @@ public void createSchema(ConnectorSession session, String schemaName, Map<String
    @Override
    public void dropSchema(ConnectorSession session, String schemaName)


The more I think about it, the less sure I am that this is the right place to delete the files.

I think it might be better to include a deleteData parameter for HiveMetastore.dropDatabase, and pass true when there are no external files. That argument is already present on HiveMetastore.dropTable and dropPartition, too, so it wouldn't be entirely out of place.

I guess if we delete the file here, though, HiveMetastore.dropDatabase should note that it's not supposed to delete non-metadata files.

jirassimok · 2021-11-08T14:36:54Z

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestCreateDropSchema.java

-        assertQuerySucceeds(format("CREATE SCHEMA %s WITH (location = '%s')", schemaName, schemaDir));
-        assertQuerySucceeds(format("DROP SCHEMA %s", schemaName));
+    @Test
+    public void testDropSchemaWithoutLocation()


I'm not completely sure how this case still works (well, it seemed to work locally; maybe it doesn't actually work). If the location property is not set, HiveMetadata shouldn't be deleting anything, and the HiveMetastore implementations shouldn't be deleting anything, either.

(Though I don't believe this test runs with every HiveMetastore we have, which would probably be good coverage to have.)

findepi · 2021-11-08T15:19:44Z

Extracted #9902 from here

findepi · 2021-11-08T15:21:20Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+                FileSystem fs = hdfsEnvironment.getFileSystem(new HdfsContext(session), path);
+                // If no files in schema directory, delete it
+                if (!fs.listFiles(path, false).hasNext()) {
+                    fs.delete(path, true);


This should be done in SemiTransactionalHiveMetastore, and only when commit is invoked

add a test with:

DROP SCHEMA

rollback

try to use that schema

findepi · 2021-11-08T15:22:24Z

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestCreateDropSchema.java

+    }
+
+    @Test
+    public void testDropSchemaDeletesDirectory()


This depends on file systems.
We should have a copy of that test in io.trino.plugin.hive.AbstractTestHive, so it's run for supported storages too.

findepi · 2021-11-09T09:02:44Z

#9902 merged, @jirassimok can you please rebase & squash?

losipiuk

First commit LGTM

losipiuk · 2021-11-17T17:46:58Z

@electrum this slightly changes the semantics of DROP SCHEMA in Hive. You may be intereseted.

jirassimok · 2021-11-18T19:47:14Z

That update added some logging.

losipiuk · 2021-11-18T22:00:35Z

Still LGTM. Maybe:

drop "WIP" commit
test Glue manually for now
fix maven checks

And we can merge this one?

jirassimok · 2021-11-18T22:21:43Z

Sounds good.

@alexjo2144 also suggested logging a warning instead of throwing an exception if there's an error while trying to delete the files, because the schema has already been dropped at that point.

In HiveMetadata, delete an empty schema location after dropping it from the metastore. In ThriftHiveMetastore and FileHiveMetastore, do not delete data.

jirassimok · 2021-11-19T20:32:25Z

Last push just fixed an copy/paste error in the tests that made one of them fail.

cla-bot bot added the cla-signed label Oct 22, 2021

jirassimok commented Oct 22, 2021

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java Outdated Show resolved Hide resolved

jirassimok requested a review from losipiuk October 22, 2021 13:37

losipiuk reviewed Oct 22, 2021

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java Outdated Show resolved Hide resolved

losipiuk reviewed Oct 22, 2021

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java Outdated Show resolved Hide resolved

losipiuk reviewed Oct 22, 2021

View reviewed changes

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestCreateDropSchema.java Outdated Show resolved Hide resolved

losipiuk reviewed Oct 22, 2021

View reviewed changes

jirassimok force-pushed the dont-drop-schemas-with-files branch 2 times, most recently from 514304f to 2d197c5 Compare October 22, 2021 16:03

jirassimok mentioned this pull request Oct 26, 2021

Disallow dropping Iceberg schema that contains external files #9767

Closed

jirassimok force-pushed the dont-drop-schemas-with-files branch from 1aa0008 to e993519 Compare October 27, 2021 21:07

jirassimok requested a review from losipiuk October 28, 2021 15:20

losipiuk reviewed Oct 29, 2021

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java Outdated Show resolved Hide resolved

losipiuk reviewed Oct 29, 2021

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java Outdated Show resolved Hide resolved

losipiuk reviewed Oct 29, 2021

View reviewed changes

jirassimok force-pushed the dont-drop-schemas-with-files branch 4 times, most recently from fc68747 to daf9981 Compare October 29, 2021 14:33

jirassimok force-pushed the dont-drop-schemas-with-files branch from daf9981 to 412d29e Compare November 4, 2021 18:52

jirassimok commented Nov 5, 2021

View reviewed changes

jirassimok commented Nov 8, 2021

View reviewed changes

findepi added the tests:hive label Nov 8, 2021

findepi mentioned this pull request Nov 8, 2021

Testing code refactors #9902

Merged

findepi reviewed Nov 8, 2021

View reviewed changes

jirassimok force-pushed the dont-drop-schemas-with-files branch 2 times, most recently from aab6657 to 1b56cc6 Compare November 15, 2021 22:57

jirassimok force-pushed the dont-drop-schemas-with-files branch from fb8d1c4 to 85a8348 Compare November 17, 2021 17:39

losipiuk reviewed Nov 17, 2021

View reviewed changes

losipiuk requested a review from electrum November 17, 2021 17:47

jirassimok force-pushed the dont-drop-schemas-with-files branch 2 times, most recently from fe78787 to 7b2e2d3 Compare November 18, 2021 19:45

jirassimok force-pushed the dont-drop-schemas-with-files branch from 7b2e2d3 to 149992b Compare November 18, 2021 22:33

Only delete non-empty directories when dropping Hive schemas

11ba2d2

In HiveMetadata, delete an empty schema location after dropping it from the metastore. In ThriftHiveMetastore and FileHiveMetastore, do not delete data.

jirassimok force-pushed the dont-drop-schemas-with-files branch from 149992b to 11ba2d2 Compare November 19, 2021 19:46

jirassimok requested a review from findepi November 19, 2021 20:31

losipiuk approved these changes Nov 20, 2021

View reviewed changes

losipiuk merged commit a89d7c1 into trinodb:master Nov 20, 2021

github-actions bot added this to the 365 milestone Nov 20, 2021

losipiuk mentioned this pull request Nov 26, 2021

Revert "Only delete non-empty directories when dropping Hive schemas" #10084

Merged

jirassimok mentioned this pull request Dec 1, 2021

Disallow dropping Hive schema that contains external files #10146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow dropping Hive schemas that contain external files #9740

Disallow dropping Hive schemas that contain external files #9740

jirassimok commented Oct 22, 2021

losipiuk left a comment

losipiuk Oct 29, 2021

jirassimok Oct 29, 2021

losipiuk Oct 29, 2021

jirassimok Oct 29, 2021

losipiuk Oct 29, 2021

losipiuk left a comment

jirassimok commented Oct 29, 2021

findepi commented Oct 30, 2021

jirassimok Nov 5, 2021

jirassimok Nov 8, 2021 •

edited

Loading

findepi commented Nov 8, 2021

findepi Nov 8, 2021 •

edited

Loading

findepi Nov 8, 2021

findepi commented Nov 9, 2021

losipiuk left a comment

losipiuk commented Nov 17, 2021

jirassimok commented Nov 18, 2021

losipiuk commented Nov 18, 2021

jirassimok commented Nov 18, 2021

jirassimok commented Nov 19, 2021

		@@ -783,7 +783,27 @@ public void createSchema(ConnectorSession session, String schemaName, Map<String
		@Override
		public void dropSchema(ConnectorSession session, String schemaName)

Disallow dropping Hive schemas that contain external files #9740

Disallow dropping Hive schemas that contain external files #9740

Conversation

jirassimok commented Oct 22, 2021

losipiuk left a comment

Choose a reason for hiding this comment

losipiuk Oct 29, 2021

Choose a reason for hiding this comment

jirassimok Oct 29, 2021

Choose a reason for hiding this comment

losipiuk Oct 29, 2021

Choose a reason for hiding this comment

jirassimok Oct 29, 2021

Choose a reason for hiding this comment

losipiuk Oct 29, 2021

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

jirassimok commented Oct 29, 2021

findepi commented Oct 30, 2021

jirassimok Nov 5, 2021

Choose a reason for hiding this comment

jirassimok Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

findepi commented Nov 8, 2021

findepi Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

findepi Nov 8, 2021

Choose a reason for hiding this comment

findepi commented Nov 9, 2021

losipiuk left a comment

Choose a reason for hiding this comment

losipiuk commented Nov 17, 2021

jirassimok commented Nov 18, 2021

losipiuk commented Nov 18, 2021

jirassimok commented Nov 18, 2021

jirassimok commented Nov 19, 2021

jirassimok Nov 8, 2021 •

edited

Loading

findepi Nov 8, 2021 •

edited

Loading