Extend Alluxio support to Iceberg connector #20602

amoghmargoor · 2024-02-06T19:17:50Z

Description

In this patch we are extending the alluxio changes which exists currently for Delta connector to also include Iceberg.

Additional context and related issues

Fixes: #19829

Part of #20550

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Iceberg
* Improve performance of scans by adding the ability to cache data files on local SSDs ({issue}`20602`)

cla-bot · 2024-02-06T19:17:53Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Amogh Margoor.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2024-02-06T19:25:45Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-06T19:45:52Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

mosabua · 2024-02-06T20:07:50Z

@cla-bot check

cla-bot · 2024-02-06T20:07:52Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-06T20:07:53Z

The cla-bot has been summoned, and re-checked this pull request!

mosabua · 2024-02-06T20:17:28Z

CLA is good now. Thanks @amoghmargoor

jkylling

In the Delta Lake connector we added a variant of the base connector test with caching

trino/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeAlluxioCacheMinioAndHmsConnectorSmokeTest.java

Line 32 in 273b535

public class TestDeltaLakeAlluxioCacheMinioAndHmsConnectorSmokeTest

, but it might be unnecessary.

I assume the product test and the Iceberg tests are still work in progress. The rest looks good.

P.S.
If it is of help, these commands were the inner part of my slow development loop when developing the product tests:

./mvnw -pl ':trino-product-tests-launcher'  install -nsu -DskipTests -Dskip.npm -Dskip.yarn -T 1C
./mvnw -pl ':trino-product-tests'  install -nsu -DskipTests -Dskip.npm -Dskip.yarn -T 1C
testing/bin/ptl test run --environment multinode-minio-data-lake-caching -- -t io.trino.tests.product.deltalake.TestDeltaLakeAlluxioCaching

You can probably extend the EnvMultinodeMinioDataLakeCaching for the product test.

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/cache/IcebergCacheKeyProvider.java

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAlluxioCacheFileOperations.java

raunaqmorarka · 2024-02-08T18:22:54Z

When I tried to benchmark this PR I ran into planning timeouts on the coordinator, it likely indicates some issue with caching during metadata reads in the coordinator. I disabled caching on coordinator in a branch and ran benchmarks on that.
[Alluxio iceberg sf1k parquet partitioned.pdf](https://github.com/trinodb/trino/fil
Alluxio iceberg sf1k parquet unpartitioned.pdf
es/14213187/Alluxio.iceberg.sf1k.parquet.partitioned.pdf)
Partitioned parquet sf1k

Unpartitioned parquet sf1k

Significant improvements in wall time and some improvement in CPU time

amoghmargoor · 2024-02-12T13:53:08Z

I have rebased it and added couple of product tests. Its still enabled on coodinator which i will try to remove now.

amoghmargoor · 2024-02-12T14:07:42Z

@raunaqmorarka I have removed the Alluxio module from being binded from Coordinator for now. however the alluxio configs needs to be binded in coodinator still else they will throw property not used exception. Take a look at that logic. there might be few product test errors - ignore that for now i will fix them.

raunaqmorarka

lgtm % minor comments

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

.../src/test/java/io/trino/plugin/iceberg/TestIcebergMinioParquetConnectorCachingSmokeTest.java

.../trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java

raunaqmorarka · 2024-02-13T11:58:22Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/cache/IcebergCacheKeyProvider.java

+    public Optional<String> getCacheKey(TrinoInputFile delegate)
+            throws IOException
+    {
+        return Optional.of(delegate.location().path());


I'm assuming that the iceberg spec guarantees that all data files in iceberg are immutable

Spec says: "Once written, data and metadata files are immutable until they are deleted".

...cker/presto-product-tests/conf/environment/multinode-iceberg-minio-cached/iceberg.properties

plugin/trino-iceberg/pom.xml

amoghmargoor changed the title ~~Enable Alluxio for Iceberg connector~~ Extend Alluxio support to Iceberg connector Feb 6, 2024

mosabua mentioned this pull request Feb 6, 2024

Managed roll out of filesystem caching support #20550

Open

mosabua requested review from electrum, wendigo, raunaqmorarka, jkylling and findepi February 6, 2024 19:24

amoghmargoor force-pushed the oss_alluxio_cache_2 branch from b0ee7bc to 7bc8331 Compare February 6, 2024 19:25

github-actions bot added the iceberg Iceberg connector label Feb 6, 2024

raunaqmorarka added the performance label Feb 6, 2024

amoghmargoor force-pushed the oss_alluxio_cache_2 branch from 7bc8331 to cbbb33f Compare February 6, 2024 19:45

amoghmargoor force-pushed the oss_alluxio_cache_2 branch from cbbb33f to b2a27e1 Compare February 6, 2024 20:12

cla-bot bot added the cla-signed label Feb 6, 2024

github-actions bot added the tests:hive label Feb 6, 2024

amoghmargoor force-pushed the oss_alluxio_cache_2 branch from b2a27e1 to ba797d3 Compare February 6, 2024 20:31

jkylling reviewed Feb 6, 2024

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/cache/IcebergCacheKeyProvider.java Outdated Show resolved Hide resolved

electrum requested changes Feb 7, 2024

View reviewed changes

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAlluxioCacheFileOperations.java Outdated Show resolved Hide resolved

amoghmargoor force-pushed the oss_alluxio_cache_2 branch from ba797d3 to 1cb491d Compare February 12, 2024 13:51

amoghmargoor force-pushed the oss_alluxio_cache_2 branch from 1cb491d to 0b5e101 Compare February 12, 2024 14:04

amoghmargoor force-pushed the oss_alluxio_cache_2 branch from 0b5e101 to c60d707 Compare February 12, 2024 14:22

github-actions bot added the delta-lake Delta Lake connector label Feb 12, 2024

amoghmargoor force-pushed the oss_alluxio_cache_2 branch 6 times, most recently from f5bde1f to 0bcfea4 Compare February 13, 2024 11:54

raunaqmorarka reviewed Feb 13, 2024

View reviewed changes

amoghmargoor force-pushed the oss_alluxio_cache_2 branch 2 times, most recently from b1d9d72 to 5783ce1 Compare February 13, 2024 14:21

findepi reviewed Feb 13, 2024

View reviewed changes

plugin/trino-iceberg/pom.xml Show resolved Hide resolved

amoghmargoor force-pushed the oss_alluxio_cache_2 branch from 5783ce1 to 1827b2a Compare February 13, 2024 16:48

mosabua mentioned this pull request Feb 13, 2024

Add docs for fs caching with Iceberg #20599

Merged

Extend Alluxio support for Iceberg connector

2c35b0e

raunaqmorarka force-pushed the oss_alluxio_cache_2 branch from 1827b2a to 2c35b0e Compare February 13, 2024 17:57

raunaqmorarka approved these changes Feb 13, 2024

View reviewed changes

raunaqmorarka requested a review from electrum February 13, 2024 17:58

raunaqmorarka merged commit 102ce9d into trinodb:master Feb 14, 2024
64 checks passed

github-actions bot added this to the 439 milestone Feb 14, 2024

colebow mentioned this pull request Feb 14, 2024

Add Trino 439 release notes #20604

Merged

takezoe mentioned this pull request Apr 21, 2024

Include addresses in retainedSize of IcebergSplit #21644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend Alluxio support to Iceberg connector #20602

Extend Alluxio support to Iceberg connector #20602

amoghmargoor commented Feb 6, 2024 •

edited by raunaqmorarka

Loading

cla-bot bot commented Feb 6, 2024

cla-bot bot commented Feb 6, 2024

cla-bot bot commented Feb 6, 2024

mosabua commented Feb 6, 2024

cla-bot bot commented Feb 6, 2024

cla-bot bot commented Feb 6, 2024

mosabua commented Feb 6, 2024

jkylling left a comment •

edited

Loading

raunaqmorarka commented Feb 8, 2024 •

edited

Loading

amoghmargoor commented Feb 12, 2024

amoghmargoor commented Feb 12, 2024

raunaqmorarka left a comment

raunaqmorarka Feb 13, 2024

amoghmargoor Feb 13, 2024

Extend Alluxio support to Iceberg connector #20602

Extend Alluxio support to Iceberg connector #20602

Conversation

amoghmargoor commented Feb 6, 2024 • edited by raunaqmorarka Loading

Description

Additional context and related issues

Release notes

cla-bot bot commented Feb 6, 2024

cla-bot bot commented Feb 6, 2024

cla-bot bot commented Feb 6, 2024

mosabua commented Feb 6, 2024

cla-bot bot commented Feb 6, 2024

cla-bot bot commented Feb 6, 2024

mosabua commented Feb 6, 2024

jkylling left a comment • edited Loading

Choose a reason for hiding this comment

raunaqmorarka commented Feb 8, 2024 • edited Loading

amoghmargoor commented Feb 12, 2024

amoghmargoor commented Feb 12, 2024

raunaqmorarka left a comment

Choose a reason for hiding this comment

raunaqmorarka Feb 13, 2024

Choose a reason for hiding this comment

amoghmargoor Feb 13, 2024

Choose a reason for hiding this comment

amoghmargoor commented Feb 6, 2024 •

edited by raunaqmorarka

Loading

jkylling left a comment •

edited

Loading

raunaqmorarka commented Feb 8, 2024 •

edited

Loading