Add a new S3FileSystemType in addition to PRESTO and EMRFS #1397

apc999 · 2019-08-28T01:06:31Z

This small PR aims to allow a storage service to serve URLs with s3 scheme. Currently, input with URLs like s3://bucket/path can only be served by PrestoS3FileSystem or EMR FS class (i.e., com.amazon.ws.emr.hadoop.fs.EmrFileSystem). In addition to these two possible choices, a RuntimeException will be thrown. This PR enables Presto to be served by additional services (e.g., Alluxio as a caching layer on top of S3 but without change HMS).

Particularly, users can update etc/config.properties

hive.s3-file-system-type=CUSTOM

and update core-site.xml

<property>
  <name>fs.s3.impl</name>
  <value>alluxio.hadoop.ShimFileSystem</value>
</property>

As a result, end users can transparently benefit from the caching from Alluxio for Presto.

Fixes #1416

cla-bot · 2019-08-28T01:06:33Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

kokosing · 2019-08-28T08:18:47Z

Why not to add additional property to Presto instead of loading hadoop specific files like core-site.xml

hive.s3-file-system-type=CUSTOM
hive.s3-file-system-type.class=alluxio.hadoop.FileSystem

Additionally, we could have something that could load file system that is configured in hadoop.

hive.s3-file-system-type=HADOOP
hive.config.resources=s3-site.xml

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/HiveS3Module.java

apc999 · 2019-08-28T19:45:06Z

Why not to add additional property to Presto instead of loading hadoop specific files like core-site.xml
hive.s3-file-system-type=CUSTOM
hive.s3-file-system-type.class=alluxio.hadoop.FileSystem
Additionally, we could have something that could load file system that is configured in hadoop.
hive.s3-file-system-type=HADOOP
hive.config.resources=s3-site.xml

Thanks for the suggestion @kokosing . I like the second approach.
This particular PR is also required to allow hive.s3-file-system-type=CUSTOM in addition to two accepted values PRESTO, EMRFS. Otherwise a RuntimeException will be thrown.

Regarding hive.config.resources=s3-site.xml, I notice that currently configuration like hive.config.resources=/<PATH>/core-site.xml,/<PATH>/hdfs-site.xml is documented. Do I need to modify anything to allow hive.config.resources=s3-site.xml?

electrum

hive.config.resources is an arbitrary list of Hadoop config files, so they can be named anything.

I had the same thought as @kokosing about configuring the class name via a normal Presto config property. However, I'm not sure if this is really needed, since the Hadoop config file is likely still need for other file system configuration. One reason to want a Presto config is so that we can ensure all three schemes, s3, s3a, s3n, are mapped to the same class name. But again, I'm not sure that is needed. Thoughts?

Otherwise, this PR looks good. @apc999 please squash the commits, then we can merge this, assuming everyone agrees that no additional config is needed.

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/HiveS3Module.java

findepi

What will be the result (error?) when user chooses CUSTOM/DEFAULT but does not provide -site.xml with the configuration for s3 filesystem?

findepi · 2019-08-28T21:00:44Z

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/S3FileSystemType.java

@@ -17,4 +17,5 @@
 {
    PRESTO,
    EMRFS,
+    CUSTOM,


@kokosing proposed "HADOOP"

I'd propose "HADOOP_DEFAULT".

Thanks for the suggestion. After a second thoughts, I will go with HADOOP for the following reasons:

This approach does go through Hadoop DFS interface to classload the correct implementing class. So HADOOP is accurate.

HADOOP_DEFAULT may remind users the other Hadoop property fs.defaultFS (link, see below) on setting a default DFS, which is not really the property used to inject the scheme like Alluxio here.

<property> <name>fs.defaultFS</name> <value>file:///</value> </property>

HADOOP_DEFAULT may remind users the other Hadoop property fs.defaultFS

I see your point.
To me it stands for "Hadoop's default".
I prefer to have this "default" there, because "Hadoop" is not a filesystem implementation.

(In fact, we internally already use HADOOP_DEFAULT with exactly same meaning (for some service other than file system))

Also it could be named NONE because we don't set anything in Presto and one may not load any resource files.

I don't like NONE, as we may end up having s3 fs impl.
@kokosing WDYT about what I suggested?

Sounds good to me.

make sense. Changed to HADOOP_DEFAULT to be consistent.

electrum · 2019-08-28T23:11:05Z

@findepi if they choose the option and do nothing else, the FS schemes will simply not exist.

apc999 · 2019-08-28T23:19:23Z

@findepi Exception like "No FileSystem for scheme: s3" will be returned.

apc999 · 2019-08-29T00:14:27Z

Commits squashed

apc999 · 2019-08-29T05:33:08Z

Some Test failed. Is it flaky?

kokosing · 2019-08-29T07:33:56Z

One reason to want a Presto config is so that we can ensure all three schemes, s3, s3a, s3n, are mapped to the same class name. But again, I'm not sure that is needed. Thoughts?

@electrum That was my intention. I agree with you that this that much useful, considering that very likely other properties have to be set as well.

This small PR aims to allow a storage service to serve URLs with s3 scheme. Currently, input with URLs like s3://bucket/path can only be served by PrestoS3FileSystem or EMR FS class (i.e., com.amazon.ws.emr.hadoop.fs.EmrFileSystem). In addition to these two possible choices, a RuntimeException will be thrown. This PR enables Presto to be served by additional services (e.g., Alluxio as a caching layer on top of S3 but without change HMS). Particularly, to leverage users can update etc/config.properties hive.s3-file-system-type=HADOOP_DEFAULT and update core-site.xml <property> <name>fs.s3.impl</name> <value>alluxio.hadoop.ShimFileSystem</value> </property>

sopel39 reviewed Aug 28, 2019

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/HiveS3Module.java Outdated Show resolved Hide resolved

sopel39 reviewed Aug 28, 2019

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/HiveS3Module.java Outdated Show resolved Hide resolved

cla-bot bot added the cla-signed label Aug 28, 2019

apc999 force-pushed the customs3 branch from f5a6dcc to 5f2e9dc Compare August 28, 2019 18:24

findepi requested a review from electrum August 28, 2019 19:41

electrum approved these changes Aug 28, 2019

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/HiveS3Module.java Outdated Show resolved Hide resolved

findepi reviewed Aug 28, 2019

View reviewed changes

apc999 force-pushed the customs3 branch from 5f2e9dc to 5d6e5de Compare August 29, 2019 00:10

apc999 force-pushed the customs3 branch from 5d6e5de to 2c44792 Compare August 29, 2019 17:14

findepi mentioned this pull request Aug 30, 2019

s3 api compatible storage #1416

Closed

sopel39 merged commit 1ab3978 into trinodb:master Aug 30, 2019

sopel39 mentioned this pull request Aug 30, 2019

Release notes for 319 #1378

Closed

6 tasks

martint added this to the 319 milestone Sep 20, 2019

findepi mentioned this pull request Sep 21, 2019

Rename S3 type HADOOP_DEFAULT to NONE #1567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new S3FileSystemType in addition to PRESTO and EMRFS #1397

Add a new S3FileSystemType in addition to PRESTO and EMRFS #1397

apc999 commented Aug 28, 2019 •

edited by findepi

Loading

cla-bot bot commented Aug 28, 2019

kokosing commented Aug 28, 2019

apc999 commented Aug 28, 2019

electrum left a comment

findepi left a comment

findepi Aug 28, 2019

apc999 Aug 28, 2019

apc999 Aug 28, 2019 •

edited

Loading

findepi Aug 29, 2019

kokosing Aug 29, 2019

findepi Aug 29, 2019

kokosing Aug 29, 2019

apc999 Aug 29, 2019

electrum commented Aug 28, 2019

apc999 commented Aug 28, 2019

apc999 commented Aug 29, 2019

apc999 commented Aug 29, 2019

kokosing commented Aug 29, 2019

@@ @@ -17,4 +17,5 @@ @@
               {
                   PRESTO,
                   EMRFS,
+                  CUSTOM,

Add a new S3FileSystemType in addition to PRESTO and EMRFS #1397

Add a new S3FileSystemType in addition to PRESTO and EMRFS #1397

Conversation

apc999 commented Aug 28, 2019 • edited by findepi Loading

cla-bot bot commented Aug 28, 2019

kokosing commented Aug 28, 2019

apc999 commented Aug 28, 2019

electrum left a comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

findepi Aug 28, 2019

Choose a reason for hiding this comment

apc999 Aug 28, 2019

Choose a reason for hiding this comment

apc999 Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

findepi Aug 29, 2019

Choose a reason for hiding this comment

kokosing Aug 29, 2019

Choose a reason for hiding this comment

findepi Aug 29, 2019

Choose a reason for hiding this comment

kokosing Aug 29, 2019

Choose a reason for hiding this comment

apc999 Aug 29, 2019

Choose a reason for hiding this comment

electrum commented Aug 28, 2019

apc999 commented Aug 28, 2019

apc999 commented Aug 29, 2019

apc999 commented Aug 29, 2019

kokosing commented Aug 29, 2019

apc999 commented Aug 28, 2019 •

edited by findepi

Loading

apc999 Aug 28, 2019 •

edited

Loading