Disallow dropping Hive schema that contains external files #10146

jirassimok · 2021-12-01T23:30:12Z

This will attempt to reimplement the behavior from #9740 in a way that doesn't break for some configurations.

findepi

i understand the missing piece is the logic which would drive the value for the deleteData parameter.

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

findepi · 2021-12-02T14:27:41Z

@jirassimok can you please verify Glue CREATE SCHEMA and CREATE SCHEMA WITH (location=..) vs DROP SCHEMA?
this can be manual, but can also be as a prep commit with a test in TestHiveGlueMetastore / AbstractTestHive

findepi · 2021-12-06T14:09:56Z

@jirassimok did you look at the CI's red?

jirassimok · 2021-12-06T15:10:18Z

It doesn't look related.

io.trino.plugin.thrift.integration.TestThriftDistributedQueriesIndexed.testHighCardinalityIndexJoinResult

java.lang.IllegalArgumentException: Expected operatorType to be PagesIndexBuilderOperator but was PageBufferOperator

.../trino-hive/src/main/java/io/trino/plugin/hive/metastore/SemiTransactionalHiveMetastore.java

losipiuk

reviewed. Tiny things + question ragarding default behaviour for case when we fail to list schema directory.

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

findepi · 2021-12-08T11:17:14Z

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

@@ -203,6 +203,12 @@ public synchronized void createDatabase(HiveIdentity identity, Database database

        Path databaseMetadataDirectory = getDatabaseMetadataDirectory(database.getDatabaseName());
        writeSchemaFile(DATABASE, databaseMetadataDirectory, databaseCodec, new DatabaseMetadata(currentVersion, database), false);
+        try {
+            metadataFileSystem.mkdirs(databaseMetadataDirectory);


Why is it needed in Don't put database schema files in the database directories commit?

It's ensuring that the database directory exists when the database is created. It's not strictly necessary, but I think it makes sense.

findepi · 2021-12-08T11:19:36Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

@@ -555,6 +562,10 @@ public void dropDatabase(HiveIdentity identity, String databaseName, boolean del
        catch (AmazonServiceException e) {
            throw new TrinoException(HIVE_METASTORE_ERROR, e);
        }
+
+        if (deleteData) {
+            location.ifPresent(path -> deleteDir(hdfsContext, hdfsEnvironment, new Path(path), true));


it's an error when deleteData && location.isEmpty().
Either checkState or add a comment why we're not failing

This can theoretically occur if database.getLocation() returns empty.

In this case, it only occurs if Glue's API returns null for the database location, which seems unlikely, but that method is not documented as non-null.

.../trino-hive/src/main/java/io/trino/plugin/hive/metastore/SemiTransactionalHiveMetastore.java

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

.../trino-hive/src/main/java/io/trino/plugin/hive/metastore/SemiTransactionalHiveMetastore.java

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

jirassimok · 2021-12-10T14:32:31Z

Green tests before new push.

jirassimok · 2021-12-10T14:35:31Z

Changes in last update: auto-close the FileSystem object and catch runtime exceptions when attempting to list files.

findepi

lgtm

losipiuk · 2021-12-14T08:41:29Z

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

@@ -93,6 +93,7 @@
    private boolean immutablePartitions;
    private Optional<InsertExistingPartitionsBehavior> insertExistingPartitionsBehavior = Optional.empty();
    private boolean createEmptyBucketFiles;
+    private boolean deleteSchemaLocationsFallback = true;


Why true as default. You probably talked this through.

It's the current behavior, so it's less likely to cause problems for anyone upgrading. But it's also the less-safe behavior for someone who isn't familiar with Trino who might , especially if they aren't very familiar with it. I think we did discussed it last week, but maybe we should change it?

What do you think, @findepi?

The point of the change is "do not delete those random files when deleting schema directory".
false feels more appropriate in case we cannot verify whether there are any files in there.
let's also add code comment discussion the rationale behind the choice we put in the default

Switched the default and added a comment.

jirassimok · 2021-12-15T19:23:41Z

Rebased forward, and avoided including new HdfsContext in the try/catch.

Instead of /catalog/database/.trinoSchema, the database schemas in FileHiveMetastore now go in /catalog/.trinoSchema.database.

findepi · 2021-12-17T08:36:56Z

@jirassimok mind the CI

In SemiTransactionalHiveMetastore, check for files before dropping the schema. Do not request deletion (via HiveMetastore) if files are visible in the schema location. A new config property, hive.delete-schema-locations-fallback, determines the behavior when Trino can't check the file location. False (the default) will not request deletion in that case.

jirassimok · 2021-12-17T20:30:34Z

All tests have passed, something is just dangling in this one so it isn't finishing the job.

cla-bot bot added the cla-signed label Dec 1, 2021

findepi added the tests:hive label Dec 2, 2021

findepi reviewed Dec 2, 2021

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java Show resolved Hide resolved

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java Outdated Show resolved Hide resolved

jirassimok force-pushed the drop-schema-file-handling branch from 811fe45 to 71571a0 Compare December 3, 2021 19:29

jirassimok force-pushed the drop-schema-file-handling branch from e773947 to c3e09be Compare December 6, 2021 19:17

losipiuk reviewed Dec 7, 2021

View reviewed changes

.../trino-hive/src/main/java/io/trino/plugin/hive/metastore/SemiTransactionalHiveMetastore.java Show resolved Hide resolved

losipiuk reviewed Dec 7, 2021

View reviewed changes

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

losipiuk reviewed Dec 7, 2021

View reviewed changes

jirassimok force-pushed the drop-schema-file-handling branch from c3e09be to e3abc7a Compare December 8, 2021 00:16

jirassimok requested a review from losipiuk December 8, 2021 00:17

findepi reviewed Dec 8, 2021

View reviewed changes

jirassimok force-pushed the drop-schema-file-handling branch from e3abc7a to 07ed54f Compare December 8, 2021 21:08

jirassimok marked this pull request as ready for review December 8, 2021 21:08

jirassimok force-pushed the drop-schema-file-handling branch 4 times, most recently from 2fd7051 to 6116ac0 Compare December 10, 2021 14:32

jirassimok force-pushed the drop-schema-file-handling branch from 6116ac0 to 73c0880 Compare December 10, 2021 14:34

findepi approved these changes Dec 13, 2021

View reviewed changes

losipiuk reviewed Dec 14, 2021

View reviewed changes

losipiuk approved these changes Dec 14, 2021

View reviewed changes

jirassimok mentioned this pull request Dec 14, 2021

Disallow dropping Iceberg schema that contains external files #9767

Closed

jirassimok force-pushed the drop-schema-file-handling branch from 73c0880 to ec3c615 Compare December 15, 2021 19:22

jirassimok force-pushed the drop-schema-file-handling branch from ec3c615 to 877cc04 Compare December 16, 2021 21:40

jirassimok added 6 commits December 16, 2021 17:59

Fix a typo

8c5601a

Pass schema type to FileHiveMetastore.getSchemaPath and its callers

ff001b9

Don't put database schema files in the database directories

907b3cc

Instead of /catalog/database/.trinoSchema, the database schemas in FileHiveMetastore now go in /catalog/.trinoSchema.database.

Rename database object-deletion method in FileHiveMetastore

2839ede

Add deleteData parameter to HiveMetastore.dropDatabase

3714cb5

Delete database location after dropping in GlueHiveMetastore

e9e4ea5

jirassimok force-pushed the drop-schema-file-handling branch from 877cc04 to 11055a1 Compare December 16, 2021 23:00

findepi changed the title ~~Handle external files when dropping Hive schemas~~ Disallow dropping Hive schema that contains external files Dec 17, 2021

jirassimok force-pushed the drop-schema-file-handling branch from 11055a1 to 703098e Compare December 17, 2021 15:08

findepi merged commit e8c5f7c into trinodb:master Dec 20, 2021

findepi mentioned this pull request Dec 20, 2021

Release notes for 367 #10301

Closed

8 tasks

github-actions bot added this to the 367 milestone Dec 20, 2021

jirassimok mentioned this pull request Jan 6, 2022

Don't delete Hive and Iceberg schema locations with nonempty subdirectories #10485

Merged

maltesander mentioned this pull request Apr 12, 2022

Provide and test trino-server-377 stackabletech/trino-operator#187

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow dropping Hive schema that contains external files #10146

Disallow dropping Hive schema that contains external files #10146

jirassimok commented Dec 1, 2021

findepi left a comment

findepi commented Dec 2, 2021

findepi commented Dec 6, 2021

jirassimok commented Dec 6, 2021

losipiuk left a comment

findepi Dec 8, 2021

jirassimok Dec 8, 2021

findepi Dec 8, 2021

jirassimok Dec 8, 2021

jirassimok commented Dec 10, 2021 •

edited

Loading

jirassimok commented Dec 10, 2021

findepi left a comment

losipiuk Dec 14, 2021

jirassimok Dec 15, 2021

findepi Dec 16, 2021

jirassimok Dec 16, 2021

jirassimok commented Dec 15, 2021

findepi commented Dec 17, 2021

jirassimok commented Dec 17, 2021

Disallow dropping Hive schema that contains external files #10146

Disallow dropping Hive schema that contains external files #10146

Conversation

jirassimok commented Dec 1, 2021

findepi left a comment

Choose a reason for hiding this comment

findepi commented Dec 2, 2021

findepi commented Dec 6, 2021

jirassimok commented Dec 6, 2021

losipiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirassimok commented Dec 10, 2021 • edited Loading

jirassimok commented Dec 10, 2021

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirassimok commented Dec 15, 2021

findepi commented Dec 17, 2021

jirassimok commented Dec 17, 2021

jirassimok commented Dec 10, 2021 •

edited

Loading