Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Alluxio file system #21603

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

JiamingMai
Copy link
Contributor

@JiamingMai JiamingMai commented Apr 18, 2024

Description

Add trino-filesystem-alluxio module to implement the new interfaces (TrinoFileSystem, TrinoInput, TrinoInputFile, TrinoOutputFile ) and support native Alluxio.

Additional context and related issues

Docs in #23269

Release notes

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

@cla-bot cla-bot bot added the cla-signed label Apr 18, 2024
@JiamingMai JiamingMai changed the title Add alluxio file system Add Alluxio filesystem Apr 18, 2024
@JiamingMai JiamingMai force-pushed the add-alluxio-file-system branch from 1aecc4c to 7e84237 Compare April 18, 2024 16:50
Copy link

cla-bot bot commented Apr 22, 2024

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 cla-bot bot removed the cla-signed label Apr 22, 2024
@mosabua mosabua requested a review from electrum April 22, 2024 20:43
Copy link

cla-bot bot commented Apr 23, 2024

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

4 similar comments
Copy link

cla-bot bot commented Apr 23, 2024

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

Copy link

cla-bot bot commented Apr 23, 2024

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

Copy link

cla-bot bot commented Apr 23, 2024

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

Copy link

cla-bot bot commented Apr 24, 2024

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

@JiamingMai JiamingMai self-assigned this Apr 24, 2024
Copy link

cla-bot bot commented Apr 24, 2024

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

4 similar comments
Copy link

cla-bot bot commented Apr 24, 2024

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

Copy link

cla-bot bot commented Apr 24, 2024

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

Copy link

cla-bot bot commented Apr 24, 2024

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

Copy link

cla-bot bot commented Apr 24, 2024

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

@JiamingMai JiamingMai force-pushed the add-alluxio-file-system branch from b724c3d to ae34ab1 Compare April 24, 2024 08:59
Copy link

cla-bot bot commented Apr 24, 2024

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

@JiamingMai JiamingMai force-pushed the add-alluxio-file-system branch from ae34ab1 to 1e86c3c Compare April 24, 2024 09:32
@cla-bot cla-bot bot added the cla-signed label Apr 24, 2024
@github-actions github-actions bot added docs hudi Hudi connector iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector bigquery BigQuery connector mongodb MongoDB connector labels Apr 24, 2024
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@jja725 jja725 force-pushed the add-alluxio-file-system branch 2 times, most recently from 41c4bc4 to 2367351 Compare September 25, 2024 22:27
@jja725 jja725 requested a review from electrum September 25, 2024 22:31
@jja725
Copy link
Member

jja725 commented Sep 25, 2024

@electrum All comments resolved, I can rebase the commits if everything looks good.

Copy link
Member

@raunaqmorarka raunaqmorarka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash commits and rebase to latest master.
Also fix the failure in check-commit

@jja725 jja725 force-pushed the add-alluxio-file-system branch from 2367351 to f2dbb56 Compare September 26, 2024 17:52
@jja725 jja725 closed this Sep 26, 2024
@jja725 jja725 reopened this Sep 26, 2024
@mosabua mosabua changed the title Add Alluxio filesystem Add Alluxio file system Sep 26, 2024
@jja725
Copy link
Member

jja725 commented Sep 26, 2024

@mosabua I believe the test failure(trino-google-sheets) is not related to the change. Do you mind helping me rerun the test ?

@mosabua
Copy link
Member

mosabua commented Sep 26, 2024

No need to rerun the build ... the google sheets failure is definitely a false alarm.

@wendigo
Copy link
Contributor

wendigo commented Sep 26, 2024

@mosabua yeah, it's super flaky recently

@raunaqmorarka raunaqmorarka merged commit 44178ea into trinodb:master Sep 27, 2024
184 of 188 checks passed
@github-actions github-actions bot added this to the 460 milestone Sep 27, 2024
@wendigo
Copy link
Contributor

wendigo commented Sep 27, 2024

Thanks @JiamingMai for a great contribution!

@mosabua
Copy link
Member

mosabua commented Sep 27, 2024

Docs are in as well now

@mosabua mosabua mentioned this pull request Sep 27, 2024
1 task
import static org.assertj.core.api.Assertions.assertThat;

@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public abstract class AbstractTestAlluxioFileSystem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need an abstract class? Can we merge into TestAlluxioFileSystem?

@Test
public void testSimplifyPath()
{
String path = "test/level0-file0";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline this variable.

String path = "test/level0-file0";
assertThat(path).isEqualTo(AlluxioUtils.simplifyPath(path));
path = "a/./b/../../c/";
assertThat("c/").isEqualTo(AlluxioUtils.simplifyPath(path));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call order is wrong. It should be

assertThat(AlluxioUtils.simplifyPath("a/./b/../../c/")).isEqualTo("c/");

Same for others.

@ebyhr

This comment was marked as resolved.

@raunaqmorarka
Copy link
Member

Looks like this PR was merged with red CI. https://github.com/trinodb/trino/actions/runs/11057611394/job/30721797373

The only failing test yesterday was the google sheets one, which is unrelated
https://github.com/trinodb/trino/actions/runs/11058479149/job/30724603118

void testContainer()
{
assertThat(ALLUXIO_MASTER_CONTAINER.isRunning()).isTrue();
assertThat(ALLUXIO_WORKER_CONTAINER.isRunning()).isTrue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation to verify containers' status here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove it in a later PR. it's mostly for verify the docker status during the test

}

@Test
public void convertToLocation()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test prefix is missing.

Please follow https://trino.io/docs/current/develop/tests.html

  • Test classes should be defined as package-private and final.
  • Test method must start with test, for example testExplain()
  • Test methods should be defined as package-private.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed iceberg Iceberg connector stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed.
Development

Successfully merging this pull request may close these issues.