Skip to content

Inline hudi code and remove hadoop dependencies#17392

Merged
electrum merged 3 commits intotrinodb:masterfrom
homar:homar/migrate_from_using_hudi_code
Jun 2, 2023
Merged

Inline hudi code and remove hadoop dependencies#17392
electrum merged 3 commits intotrinodb:masterfrom
homar:homar/migrate_from_using_hudi_code

Conversation

@homar
Copy link
Copy Markdown
Member

@homar homar commented May 8, 2023 •
edited
Loading

Description

This PR migrates hudi connector away from hudi libraries.
It is splitted into a general preparatory commit and 3 commits that migrates specific parts of hudi connector.
Because all of these classes were very interconnected it was not possible to split them into smaller, closer parts without breaking the functionality.
Big parts of the code was directly inlined from hudi libraries.
I didn't do refactoring that was not crucial to make the code work.
If you believe that this should be done please point it out.

Additional context and related issues

Release notes

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

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label May 8, 2023
@github-actions github-actions bot added the hudi Hudi connector label May 8, 2023
@homar homar force-pushed the homar/migrate_from_using_hudi_code branch 6 times, most recently from 5f0ac6c to 0c86328 Compare May 14, 2023 10:47
@homar homar marked this pull request as ready for review May 14, 2023 11:17
@homar homar requested a review from anusudarsan May 15, 2023 16:00
@homar homar changed the title Homar/migrate from using hudi code Inline hudi code and remove hadoop dependencies May 15, 2023
@anusudarsan anusudarsan requested a review from electrum May 15, 2023 18:32
anusudarsan
anusudarsan reviewed May 15, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@anusudarsan anusudarsan left a comment

Choose a reason for hiding this comment

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

I reviewed half of it. In general I think we should stick with trino coding standards (including formatting style) in the commits. I have commented on some of the classes (like access modifiers of variables, requireNonNull checks etc), lets make sure the entire PR follows the same.

plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/table/HoodieTableMetaClient.java Outdated
Copy link
Copy Markdown
Member

@anusudarsan anusudarsan May 15, 2023

Choose a reason for hiding this comment

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

remove this TODO

plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/files/FSUtils.java Outdated
Copy link
Copy Markdown
Member

@anusudarsan anusudarsan May 15, 2023

Choose a reason for hiding this comment

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

can we just use Location.appendSuffix here?

Copy link
Copy Markdown
Member Author

@homar homar May 17, 2023

Choose a reason for hiding this comment

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

actually we can't, it doesn't add '/' and was causing test failure that i was debugging for few hours ;)

plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/table/HoodieTableFileSystemView.java Outdated
Copy link
Copy Markdown
Member

@anusudarsan anusudarsan May 15, 2023

Choose a reason for hiding this comment

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

private

plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/table/HoodieTableFileSystemView.java Outdated
Copy link
Copy Markdown
Member

@anusudarsan anusudarsan May 15, 2023

Choose a reason for hiding this comment

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

reformat the set calls to be in new line and more readable

plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/table/HoodieTableFileSystemView.java Outdated
Copy link
Copy Markdown
Member

@anusudarsan anusudarsan May 15, 2023

Choose a reason for hiding this comment

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

redundant else

plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/table/HoodieTableFileSystemView.java Outdated
Copy link
Copy Markdown
Member

@anusudarsan anusudarsan May 15, 2023

Choose a reason for hiding this comment

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

can you reformat this class ?

Copy link
Copy Markdown
Member Author

@homar homar May 16, 2023

Choose a reason for hiding this comment

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

What do you mean by reformat? I did use intellij reformat action, if you meant refactor then sure, I can try to simplify it a little

Copy link
Copy Markdown
Member Author

@homar homar May 16, 2023

Choose a reason for hiding this comment

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

Ok I did some manual reformatting of very ugly parts

@homar homar force-pushed the homar/migrate_from_using_hudi_code branch 3 times, most recently from c1d516d to 27c2d99 Compare May 17, 2023 15:10
@electrum
Copy link
Copy Markdown
Member

electrum commented May 17, 2023

Nit: fix capitalization in commit title

Remove Hadoop dependencies from Hudi

@homar homar force-pushed the homar/migrate_from_using_hudi_code branch from 27c2d99 to 785db54 Compare May 17, 2023 20:53
electrum
electrum reviewed May 18, 2023
View reviewed changes
@electrum
Copy link
Copy Markdown
Member

electrum commented May 18, 2023

Is everything imported from Hudi needed? We only need to pass the existing tests.

@homar homar force-pushed the homar/migrate_from_using_hudi_code branch from 785db54 to 9ad58a1 Compare May 18, 2023 12:49
@homar homar force-pushed the homar/migrate_from_using_hudi_code branch 2 times, most recently from 49e7f35 to 282eb7d Compare May 18, 2023 16:40
@homar homar requested a review from electrum May 18, 2023 16:42
anusudarsan and others added 2 commits May 18, 2023 18:52
@anusudarsan @homar
Remove Hudi dependencies, inline their code
9922df5 
Co-Authored-By: Konrad Dziedzic <konraddziedzic@gmail.com>
@homar
Remove Hadoop dependencies from Hudi
64bc4da
@homar homar force-pushed the homar/migrate_from_using_hudi_code branch from 282eb7d to 64bc4da Compare May 18, 2023 16:53
@homar
Copy link
Copy Markdown
Member Author

homar commented May 19, 2023

Is everything imported from Hudi needed? We only need to pass the existing tests.

Yes I think so, I tried to remove as much code as possible

@electrum electrum mentioned this pull request Jun 2, 2023
Closed
@electrum electrum merged commit 7da7882 into trinodb:master Jun 2, 2023
@github-actions github-actions bot added this to the 419 milestone Jun 2, 2023
@homar homar mentioned this pull request Jun 5, 2023
Closed
@colebow colebow mentioned this pull request Jun 5, 2023
Merged
@findepi findepi mentioned this pull request Jun 14, 2023
Merged
@codope codope mentioned this pull request Oct 20, 2023
Merged
@yihua yihua mentioned this pull request Nov 30, 2023
Merged
@agrueneberg agrueneberg mentioned this pull request Dec 8, 2023
Merged
@ebyhr ebyhr mentioned this pull request Jun 6, 2024
Open
This was referenced Nov 30, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@electrum electrum electrum approved these changes

+1 more reviewer

@anusudarsan anusudarsan anusudarsan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed hudi Hudi connector

Milestone

419

Development

Successfully merging this pull request may close these issues.

3 participants

@homar @electrum @anusudarsan