Disallow dropping Iceberg schema that contains external files #9767

jirassimok · 2021-10-26T02:34:19Z

Based on #10146
Adds same logic for Iceberg

jirassimok · 2021-10-28T21:07:19Z

The tests previously failed because Tempto thought the tests were using HDP2 (from config-default), but the Spark environments used HDP3 instead, which uses a different port, so the HdfsClient was unable to operate correctly.

To fix this, I've made the Spark environments use the same Tempto configuration as singlenode-hdp3.

...er/src/main/java/io/trino/tests/product/launcher/env/environment/EnvSinglenodeSparkHive.java

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

Shouldn't be merged in current shape -- #9740 (comment)

jirassimok · 2021-11-24T18:32:19Z

Fixed the tests; this should work properly now.

...src/main/java/io/trino/tests/product/launcher/env/environment/EnvSinglenodeSparkIceberg.java

findepi · 2021-11-24T19:09:43Z

...src/main/java/io/trino/tests/product/launcher/env/environment/EnvSinglenodeSparkIceberg.java

+        builder.configureContainer(TESTS, dockerContainer -> {
+            dockerContainer.withCopyFileToContainer(
+                    forHostPath(dockerFiles.getDockerFilesHostPath("conf/tempto/tempto-configuration-for-hive3.yaml")),
+                    CONTAINER_TEMPTO_PROFILE_CONFIG);


CONTAINER_TEMPTO_PROFILE_CONFIG is deprecated. what's the current way to accomplish this?

It lists EnvironmentContainers.configureTempto as its replacement, but that method doesn't work in this case (because that only works when the tempto configuration has a specific name and is in the environment's config directory).

Quite a few things around the environment configuration need to be refactored, and I avoided changing it as much as possible.

(This is also copied verbatim from EnvSinglenodeHdp3, so if we change it here, we should probably also change it there.)

so CONTAINER_TEMPTO_PROFILE_CONFIG is deprecated but we cannot use the replacement?
was it deprecated too early? cc @kokosing

The replacement works in most cases, but there are a few where it doesn't. To address that, we should probably add an overload like configureTempto(Environment.Builder, String) to specify which file to use rather than taking a file from a ResourceProvider.

...src/main/java/io/trino/tests/product/launcher/env/environment/EnvSinglenodeSparkIceberg.java

...g/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestCreateDropSchema.java

findepi · 2021-11-24T19:14:13Z

...g/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestCreateDropSchema.java

+
+    private void useIceberg()
+    {
+        onTrino().executeQuery("USE iceberg.default");


I'd use fully qualified names instead, but whatever

I had that at first, but then if I (or any future developer) forgot to qualify the names anywhere, the tests might pass without actually using Iceberg.

It also makes the variable declarations in the tests a little less nice, because you need the unqualified schema name for the default schema location.

I had that at first, but then if I (or any future developer) forgot to qualify the names anywhere, the tests might pass without actually using Iceberg.

we should remove default catalog/schema from tempto configuration here

trino/testing/trino-product-tests/src/main/resources/tempto-configuration.yaml

Line 34 in 4f67c90

jdbc_url: jdbc:trino://${databases.presto.host}:${databases.presto.port}/hive/${databases.hive.schema}

@jirassimok can you work on that, separately?

findepi · 2021-11-25T11:11:34Z

please add an escape hatch like #10067

jirassimok · 2021-12-14T15:51:12Z

Updated to be based on #10146. Only the last two commits are part of this PR.

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoCatalogFactory.java

findinpath · 2021-12-15T06:16:25Z

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

+            metadataFileSystem.mkdirs(databaseMetadataDirectory);
+        }
+        catch (IOException e) {
+            throw new TrinoException(HIVE_METASTORE_ERROR, "Could not write database", e);


Could you please add the database information in the exception?

That would go in #10146, but this was just copied from the other "could not write" errors in the class, which also don't give more detailed messages.

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoHiveCatalog.java

homar

LGTM

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

findepi

two last commits LGTM, rest is #10146, but not the latest version, so #10146 needs to be merged and this one rebased.

findepi · 2021-12-17T08:38:29Z

...src/main/java/io/trino/tests/product/launcher/env/environment/EnvSinglenodeSparkIceberg.java

+        builder.configureContainer(TESTS, dockerContainer -> {
+            dockerContainer.withCopyFileToContainer(
+                    forHostPath(dockerFiles.getDockerFilesHostPath("conf/tempto/tempto-configuration-for-hive3.yaml")),
+                    CONTAINER_TEMPTO_PROFILE_CONFIG);


so CONTAINER_TEMPTO_PROFILE_CONFIG is deprecated but we cannot use the replacement?
was it deprecated too early? cc @kokosing

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoHiveCatalog.java

findepi · 2021-12-17T08:40:57Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoHiveCatalog.java

+                .map(Path::new);
+
+        // If we see files in the schema location, don't delete it.
+        // If we see no files or can't see the location at all, use fallback.


If we see no files

then we delete the directory

or can't see the location at all, use fallback.

only on this latter case we use fallback

did you mean "when location is not set"?

If we see no files, delete. If we can't see or if getDatabase didn't get the location (it always should in practice, even when using default location) use fallback. I'll update the comment.

findepi · 2021-12-17T08:44:18Z

...g/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestCreateDropSchema.java

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
+
+public class TestCreateDropSchema


How should we test this logic with respect to upcoming Iceberg Glue support? (#10151)
No change requested in this PR, but still worth discussing.
cc @jirassimok @losipiuk @jackye1995 @phd3

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.

findepi · 2021-12-20T15:57:31Z

merged as ccee7b6, thanks

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

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch 3 times, most recently from 1153b1a to c16e96a Compare October 28, 2021 20:41

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch 5 times, most recently from d6c81ff to 3b2c26c Compare October 28, 2021 21:19

jirassimok commented Oct 28, 2021

View reviewed changes

...er/src/main/java/io/trino/tests/product/launcher/env/environment/EnvSinglenodeSparkHive.java Outdated Show resolved Hide resolved

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch from 3b2c26c to 1f2ae81 Compare October 29, 2021 21:22

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch from 1f2ae81 to e427f53 Compare November 1, 2021 13:23

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch from e427f53 to 5e53606 Compare November 17, 2021 23:26

jirassimok marked this pull request as ready for review November 22, 2021 14:04

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch 4 times, most recently from b36273e to 2e7df7f Compare November 24, 2021 17:55

jirassimok requested review from losipiuk and findepi November 24, 2021 18:32

findepi reviewed Nov 24, 2021

View reviewed changes

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch 2 times, most recently from 4a883f2 to ebe31db Compare November 24, 2021 20:07

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch from ebe31db to bae9114 Compare December 14, 2021 15:50

jirassimok commented Dec 14, 2021

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoCatalogFactory.java Show resolved Hide resolved

findinpath reviewed Dec 15, 2021

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoHiveCatalog.java Show resolved Hide resolved

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch from db6f0bc to d5970bb Compare December 15, 2021 19:24

homar approved these changes Dec 16, 2021

View reviewed changes

losipiuk approved these changes Dec 16, 2021

View reviewed changes

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch from d5970bb to 497fa82 Compare December 16, 2021 21:46

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 dont-drop-iceberg-schemas-with-files branch from 497fa82 to 6199026 Compare December 16, 2021 22:59

findepi reviewed Dec 17, 2021

View reviewed changes

findepi changed the title ~~Disallow dropping Iceberg schemas that contain external files~~ Disallow dropping Iceberg schema that contains external files Dec 17, 2021

jirassimok added 2 commits December 17, 2021 10:07

Use Hive 3 Tempto config in Spark product test envs

a1cfa52

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch from 6199026 to d65ec83 Compare December 17, 2021 15:08

Only delete non-empty directories when dropping Iceberg schemas

c666720

jirassimok force-pushed the dont-drop-iceberg-schemas-with-files branch from d65ec83 to c666720 Compare December 17, 2021 19:07

findepi closed this Dec 20, 2021

findepi added this to the 367 milestone Dec 20, 2021

findepi mentioned this pull request Dec 20, 2021

Release notes for 367 #10301

Closed

8 tasks

jirassimok mentioned this pull request Jan 6, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow dropping Iceberg schema that contains external files #9767

Disallow dropping Iceberg schema that contains external files #9767

jirassimok commented Oct 26, 2021 •

edited by losipiuk

Loading

jirassimok commented Oct 28, 2021 •

edited

Loading

findepi commented Oct 30, 2021

jirassimok commented Nov 24, 2021

findepi Nov 24, 2021

jirassimok Nov 24, 2021

findepi Dec 17, 2021

jirassimok Dec 17, 2021

findepi Nov 24, 2021

jirassimok Nov 24, 2021

findepi Nov 25, 2021 •

edited

Loading

findepi commented Nov 25, 2021

jirassimok commented Dec 14, 2021

findinpath Dec 15, 2021

jirassimok Dec 15, 2021

homar left a comment

findepi left a comment

findepi Dec 17, 2021

findepi Dec 17, 2021

jirassimok Dec 17, 2021

findepi Dec 17, 2021

findepi commented Dec 20, 2021

Disallow dropping Iceberg schema that contains external files #9767

Disallow dropping Iceberg schema that contains external files #9767

Conversation

jirassimok commented Oct 26, 2021 • edited by losipiuk Loading

jirassimok commented Oct 28, 2021 • edited Loading

findepi commented Oct 30, 2021

jirassimok commented Nov 24, 2021

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

Choose a reason for hiding this comment

findepi Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

findepi commented Nov 25, 2021

jirassimok commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homar left a comment

Choose a reason for hiding this comment

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

findepi commented Dec 20, 2021

jirassimok commented Oct 26, 2021 •

edited by losipiuk

Loading

jirassimok commented Oct 28, 2021 •

edited

Loading

findepi Nov 25, 2021 •

edited

Loading