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.

Sign up for GitHub

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

Jump to bottom

[HUDI-2757] Implement Hudi AWS Glue sync #5076

Merged
merged 7 commits into from
Mar 28, 2022
Merged

[HUDI-2757] Implement Hudi AWS Glue sync #5076

merged 7 commits into from
Mar 28, 2022

Conversation

xushiyan
Copy link
Member

@xushiyan xushiyan commented Mar 20, 2022 •
edited
Loading

Add AWS Glue sync implementation to allow sync to AWS Glue catalog directly via AWS SDK APIs.

@xushiyan xushiyan mentioned this pull request Mar 20, 2022
Closed
@xushiyan
Copy link
Member Author

Screen Shot 2022-03-21 at 10 48 00 PM

Verified running meta sync with org.apache.hudi.aws.sync.AwsGlueCatalogSyncTool and sync'ed to Glue table.

@apache apache deleted a comment from hudi-bot Mar 21, 2022
nsivabalan
nsivabalan reviewed Mar 21, 2022
View reviewed changes
Copy link
Contributor

@nsivabalan nsivabalan left a comment

Choose a reason for hiding this comment

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

few minor comments. looks good mostly.

hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/AbstractHiveSyncHoodieClient.java Show resolved Hide resolved
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/AbstractHiveSyncHoodieClient.java Show resolved Hide resolved
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogClient.java Outdated Show resolved Hide resolved
@nsivabalan nsivabalan self-assigned this Mar 21, 2022
@xushiyan xushiyan force-pushed the HUDI-2757-glue-sync branch 2 times, most recently from 5b94cea to a115917 Compare March 26, 2022 11:54
@xushiyan xushiyan marked this pull request as ready for review March 26, 2022 11:54
nsivabalan
nsivabalan reviewed Mar 27, 2022
View reviewed changes
Copy link
Contributor

@nsivabalan nsivabalan left a comment

Choose a reason for hiding this comment

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

So, we can't write any tests for AWSGlueCatalogSync is it. if we mock, we have to mock pretty much everything and there is no point in testing only. is my understanding right.

mostly minor comments. once addressed, we can land.

hudi-aws/src/main/java/org/apache/hudi/aws/sync/AwsGlueCatalogSyncTool.java Show resolved Hide resolved
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/AbstractHiveSyncHoodieClient.java Show resolved Hide resolved
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/AbstractHiveSyncHoodieClient.java Show resolved Hide resolved
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogClient.java Outdated Show resolved Hide resolved
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogClient.java Outdated Show resolved Hide resolved
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogClient.java Outdated Show resolved Hide resolved
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogClient.java Outdated
}

@Override
public void updateLastCommitTimeSynced(String tableName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A general question about updating HOODIE_LAST_COMMIT_TIME_SYNC. I see in HiveSyncTool, we call hoodieHiveClient.updateLastCommitTimeSynced(tableName)
in the end.
But updating partitions and updating the last commit time synced is not atomic. and within updateLastCommitTimeSynced, we get the latest commit time from activetimeline and update it as the lastCommitTimeSycned.
So, incase of multi-writers, there are chances that timeline has moved between updating partitions and calling updateLastCommitTimeSynced.

may be we should file a jira and do a follow up on the logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

yea, should be a follow up item

@xushiyan xushiyan mentioned this pull request Mar 28, 2022
Merged
nsivabalan
nsivabalan approved these changes Mar 28, 2022
View reviewed changes
Copy link
Contributor

@nsivabalan nsivabalan left a comment

Choose a reason for hiding this comment

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

LGTM

@rmahindra123
Copy link
Contributor

lgtm. Approved.

@hudi-bot
Copy link

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@nsivabalan nsivabalan merged commit 6ccbae4 into apache:master Mar 28, 2022
@xushiyan xushiyan deleted the HUDI-2757-glue-sync branch March 29, 2022 13:50
vingov pushed a commit to vingov/hudi that referenced this pull request Apr 3, 2022
@xushiyan
[HUDI-2757] Implement Hudi AWS Glue sync (apache#5076)
fe60f2c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nsivabalan nsivabalan nsivabalan approved these changes

Assignees

@nsivabalan nsivabalan

Labels
aws-support priority:blocker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xushiyan @rmahindra123 @hudi-bot @nsivabalan