Disable legacy filesystem implementation by default #23343

anusudarsan · 2024-09-09T20:57:05Z

Description

Additional context and related issues

Docs must be ready and merged at the same time so change lands in a release with the required docs. This is a breaking change for most users.

#23366

Release notes

(x) Release notes are required, with the following suggested text:

## Hive,  Delta Lake, Iceberg, and Hudi connector

* {{breaking}} Deactivate legacy file system support for all catalogs. You must 
  activate the desired [file system support](file-system-configuration) with 
  `fs.native-azure.enabled`,`fs.native-gcs.enabled`, `fs.native-s3.enabled`, or 
  `fs.hadoop.enabled` in each catalog. Use the migration guides for Azure Storage, 
  Google Cloud Storage, and S3 to assist if you have not switched from legacy support.

and link to migration guides

lib/trino-filesystem-manager/src/main/java/io/trino/filesystem/manager/FileSystemConfig.java

mosabua · 2024-09-09T21:42:20Z

Looks like they're a whole bunch of failures that I'm not false alarms @anusudarsan

mosabua · 2024-09-10T17:00:58Z

I assume you will have to update a whole bunch of catalog properties files used for testing and stuff like that @anusudarsan

mosabua · 2024-09-10T17:02:01Z

fyi @jhlodin and @Joelg96 .. you can work on docs PR in parallel

mosabua · 2024-09-10T17:05:38Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/HiveQueryRunner.java

@@ -222,6 +222,9 @@ public DistributedQueryRunner build()
                    hiveProperties.put("hive.metastore", "file");
                    hiveProperties.put("hive.metastore.catalog.dir", queryRunner.getCoordinator().getBaseDataDir().resolve("hive_data").toString());
                }
+                if (!hiveProperties.buildOrThrow().containsKey("fs.hadoop.enabled")) {


Should we not use the new navy file systems for most of the tests?

All those tests were already migrated to native-fs (including product-tests), by explicitly setting fs.hadoop.enabled to false, and enabling one of s3/azure/gcs implementations

So why do we need to add this?

@electrum there are still tests with no fs.hadoop.enabled values set. When default was changed to false, tests fail with errors like No factory set for location /tmp.. . Hence this change

Should we change those tests to make sure they do have the fs.hadoop.enabled set to on instead

Should we change those tests to make sure they do have the fs.hadoop.enabled set to on instead

We could, but there will be a lot. I followed the pattern in the runner where we default to file metastore when no metastore is set.

When default was changed to false, tests fail with errors like No factory set for location /tmp..

Here is a stacktrace

stacktrace
io.trino.spi.TrinoException: Could not read database schema
at io.trino.plugin.hive.metastore.file.FileHiveMetastore.readFile(FileHiveMetastore.java:1406) at io.trino.plugin.hive.metastore.file.FileHiveMetastore.readSchemaFile(FileHiveMetastore.java:1391) at io.trino.plugin.hive.metastore.file.FileHiveMetastore.getDatabase(FileHiveMetastore.java:285) at io.trino.plugin.hive.metastore.file.FileHiveMetastore.createDatabase(FileHiveMetastore.java:198) at io.trino.metastore.tracing.TracingHiveMetastore.lambda$createDatabase$9(TracingHiveMetastore.java:173) at io.trino.metastore.tracing.Tracing.lambda$withTracing$0(Tracing.java:31) at io.trino.metastore.tracing.Tracing.withTracing(Tracing.java:39) at io.trino.metastore.tracing.Tracing.withTracing(Tracing.java:30) at io.trino.metastore.tracing.TracingHiveMetastore.createDatabase(TracingHiveMetastore.java:173) at io.trino.plugin.hive.metastore.cache.CachingHiveMetastore.createDatabase(CachingHiveMetastore.java:572) at io.trino.plugin.deltalake.TestDeltaLakeProjectionPushdownPlans.createPlanTester(TestDeltaLakeProjectionPushdownPlans.java:119)

It looks to me that we need to have a mapping for io.trino.filesystem.local.LocalFileSystemFactory in FileSystemModule or an extension of FileSystemModule that we are using for testing purposes in order to cope with local fs locations.

Supporting local fs would be cool anyway

electrum · 2024-09-11T01:38:34Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/HiveQueryRunner.java

@@ -222,6 +222,9 @@ public DistributedQueryRunner build()
                    hiveProperties.put("hive.metastore", "file");
                    hiveProperties.put("hive.metastore.catalog.dir", queryRunner.getCoordinator().getBaseDataDir().resolve("hive_data").toString());
                }
+                if (!hiveProperties.buildOrThrow().containsKey("fs.hadoop.enabled")) {


So why do we need to add this?

jhlodin · 2024-09-11T19:21:44Z

Release note edit, suggest
"have to now explicitly enable the filesystem implementation" -> "have to now explicitly enable the desired filesystem implementation" with "filesystem implementation" linking to here - https://trino.io/docs/current/object-storage.html#configuration

Reason being, we should make it clear that there is a choice between multiple different options. As written now, it sounds like we expect users to simply re-enable the hadoop FS they were previously using.

mosabua · 2024-09-11T21:44:33Z

@anusudarsan @electrum I'm wondering if for a lot of the tests instead of defaulting to the Hadoop filesystem we should default to another one to make the removal and migration later easier. But maybe that can also be a follow-up pull request

mosabua · 2024-09-12T21:16:23Z

/test-with-secrets sha=7c60795d814d6b7d1ec2e1d3f0a4118e491d2c98

github-actions · 2024-09-12T21:25:08Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/10838968944

mosabua · 2024-09-12T23:31:42Z

/test-with-secrets sha=13ff61e6f20bb1c208d9dae0d72a55a677335da2

github-actions · 2024-09-12T23:32:46Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/10840306232

mosabua · 2024-09-13T02:31:40Z

/test-with-secrets sha=58f28d1e02179c4b0c4e3b4ae37192cd94b27e7f

github-actions · 2024-09-13T02:32:47Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/10842020393

mosabua · 2024-09-13T15:30:31Z

/test-with-secrets sha=494c0ae41b2cca6ea43d59c445027f56d323b900

github-actions · 2024-09-13T15:33:51Z

The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/10851945727

This means we disable all filesystem implementations by default, enforcing users to explicitly configure either legacy or native filesystem implementation

anusudarsan · 2024-09-13T18:28:11Z

rebased the PR

mosabua · 2024-09-13T20:05:32Z

CI is good. Failure is a flaky false alarm.

mosabua

Nice work down the rabbit hole.

ebyhr · 2024-09-13T20:46:33Z

@anusudarsan @mosabua Could you confirm CI failure? https://github.com/trinodb/trino/actions/runs/10855433827/job/30128033072

mosabua · 2024-09-13T21:15:16Z

Dang .. that passed on the PR on the runs with secrets

cla-bot bot added the cla-signed label Sep 9, 2024

anusudarsan commented Sep 9, 2024

View reviewed changes

lib/trino-filesystem-manager/src/main/java/io/trino/filesystem/manager/FileSystemConfig.java Outdated Show resolved Hide resolved

anusudarsan requested a review from electrum September 9, 2024 21:12

anusudarsan force-pushed the anu/enable-nfs-default branch 2 times, most recently from b650ce3 to 56a5e63 Compare September 10, 2024 17:00

github-actions bot added jdbc Relates to Trino JDBC driver hive Hive connector labels Sep 10, 2024

mosabua reviewed Sep 10, 2024

View reviewed changes

anusudarsan force-pushed the anu/enable-nfs-default branch from 56a5e63 to f5eed0b Compare September 10, 2024 18:51

github-actions bot added iceberg Iceberg connector delta-lake Delta Lake connector labels Sep 10, 2024

anusudarsan changed the title ~~Enable native filesystem implementation by default~~ Disable legacy filesystem implementation by default Sep 10, 2024

electrum approved these changes Sep 11, 2024

View reviewed changes

mosabua marked this pull request as ready for review September 11, 2024 18:49

anusudarsan force-pushed the anu/enable-nfs-default branch from f5eed0b to 0aae449 Compare September 11, 2024 18:51

anusudarsan force-pushed the anu/enable-nfs-default branch 2 times, most recently from 1bf8df1 to 8f8ab99 Compare September 11, 2024 21:28

mosabua mentioned this pull request Sep 11, 2024

Docs changes for Hadoop FS default #23366

Merged

anusudarsan force-pushed the anu/enable-nfs-default branch 2 times, most recently from a8b6215 to 7c60795 Compare September 12, 2024 13:29

anusudarsan force-pushed the anu/enable-nfs-default branch from 7c60795 to 13ff61e Compare September 12, 2024 22:26

anusudarsan force-pushed the anu/enable-nfs-default branch from 13ff61e to 58f28d1 Compare September 13, 2024 00:37

anusudarsan force-pushed the anu/enable-nfs-default branch from 58f28d1 to 494c0ae Compare September 13, 2024 13:07

Disable legacy filesystem implementation by default

22e84c4

This means we disable all filesystem implementations by default, enforcing users to explicitly configure either legacy or native filesystem implementation

anusudarsan force-pushed the anu/enable-nfs-default branch from 494c0ae to 22e84c4 Compare September 13, 2024 18:27

mosabua approved these changes Sep 13, 2024

View reviewed changes

mosabua merged commit 3d66499 into trinodb:master Sep 13, 2024
56 of 57 checks passed

github-actions bot added this to the 458 milestone Sep 13, 2024

anusudarsan deleted the anu/enable-nfs-default branch September 13, 2024 20:44

mosabua mentioned this pull request Sep 14, 2024

Add Trino 458 release notes #23367

Merged

guyco33 mentioned this pull request Sep 24, 2024

Unable to load credentials from any of the providers in the chain file with native s3 support #23545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable legacy filesystem implementation by default #23343

Disable legacy filesystem implementation by default #23343

anusudarsan commented Sep 9, 2024 •

edited by mosabua

Loading

mosabua commented Sep 9, 2024

mosabua commented Sep 10, 2024

mosabua commented Sep 10, 2024

mosabua Sep 10, 2024

anusudarsan Sep 10, 2024 •

edited

Loading

mosabua Sep 10, 2024

electrum Sep 11, 2024

anusudarsan Sep 11, 2024

mosabua Sep 11, 2024

anusudarsan Sep 11, 2024

findinpath Sep 12, 2024 •

edited

Loading

mosabua Sep 12, 2024

electrum Sep 11, 2024

jhlodin commented Sep 11, 2024

mosabua commented Sep 11, 2024

mosabua commented Sep 12, 2024

github-actions bot commented Sep 12, 2024 •

edited

Loading

mosabua commented Sep 12, 2024

github-actions bot commented Sep 12, 2024 •

edited

Loading

mosabua commented Sep 13, 2024

github-actions bot commented Sep 13, 2024 •

edited

Loading

mosabua commented Sep 13, 2024

github-actions bot commented Sep 13, 2024

anusudarsan commented Sep 13, 2024

mosabua commented Sep 13, 2024

mosabua left a comment

ebyhr commented Sep 13, 2024

mosabua commented Sep 13, 2024

Disable legacy filesystem implementation by default #23343

Disable legacy filesystem implementation by default #23343

Conversation

anusudarsan commented Sep 9, 2024 • edited by mosabua Loading

Description

Additional context and related issues

Release notes

mosabua commented Sep 9, 2024

mosabua commented Sep 10, 2024

mosabua commented Sep 10, 2024

Choose a reason for hiding this comment

anusudarsan Sep 10, 2024 • edited Loading

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

findinpath Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhlodin commented Sep 11, 2024

mosabua commented Sep 11, 2024

mosabua commented Sep 12, 2024

github-actions bot commented Sep 12, 2024 • edited Loading

mosabua commented Sep 12, 2024

github-actions bot commented Sep 12, 2024 • edited Loading

mosabua commented Sep 13, 2024

github-actions bot commented Sep 13, 2024 • edited Loading

mosabua commented Sep 13, 2024

github-actions bot commented Sep 13, 2024

anusudarsan commented Sep 13, 2024

mosabua commented Sep 13, 2024

mosabua left a comment

Choose a reason for hiding this comment

ebyhr commented Sep 13, 2024

mosabua commented Sep 13, 2024

anusudarsan commented Sep 9, 2024 •

edited by mosabua

Loading

anusudarsan Sep 10, 2024 •

edited

Loading

findinpath Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 13, 2024 •

edited

Loading