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

[GOBBLIN-1802]Register iceberg table metadata update with destination side catalog #3663

Merged
merged 9 commits into from
Apr 3, 2023
Merged

[GOBBLIN-1802]Register iceberg table metadata update with destination side catalog #3663

merged 9 commits into from
Apr 3, 2023

Conversation

meethngala
Copy link
Contributor

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

Implementing the capability to register iceberg tables based on table metadata updates after the data has been published. Below are the changes as part of this PR:

  • Here are some details about my PR, including screenshots (if applicable):
  • Accept properties for instantiating both source and target IcebergCatalog
  • Added IcebergRegisterStep to perform post publish CommitStep
  • Extended IcebergTable class to support registerIcebergTable which relies on the underlying TableOperations'scommit method for registration

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
  • Updated unit tests

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

phet
phet reviewed Mar 17, 2023
View reviewed changes
...management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDataset.java Outdated Show resolved Hide resolved
...management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDataset.java Outdated Show resolved Hide resolved
...management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDataset.java Outdated Show resolved Hide resolved
...management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDataset.java Outdated Show resolved Hide resolved
...management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDataset.java Outdated Show resolved Hide resolved
...ment/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetFinder.java Outdated Show resolved Hide resolved
...ement/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergRegisterStep.java Outdated Show resolved Hide resolved
...gement/src/test/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetTest.java Outdated Show resolved Hide resolved
...gement/src/test/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetTest.java Show resolved Hide resolved
...gement/src/test/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetTest.java Outdated Show resolved Hide resolved
phet
phet reviewed Mar 22, 2023
View reviewed changes
Copy link
Contributor

@phet phet left a comment

Choose a reason for hiding this comment

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

very close!

...management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDataset.java Outdated Show resolved Hide resolved
...ment/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetFinder.java Outdated Show resolved Hide resolved
...ment/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetFinder.java Show resolved Hide resolved
...gement/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergHiveCatalog.java Outdated Show resolved Hide resolved
...ement/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergRegisterStep.java Outdated Show resolved Hide resolved
...management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDataset.java Outdated Show resolved Hide resolved
...gement/src/test/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetTest.java Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2023 •
edited
Loading

Codecov Report

Merging #3663 (7fbf7f1) into master (be6d46c) will decrease coverage by 2.17%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #3663      +/-   ##
============================================
- Coverage     46.85%   44.68%   -2.17%     
+ Complexity    10749     2088    -8661     
============================================
  Files          2138      411    -1727     
  Lines         83989    17697   -66292     
  Branches       9331     2157    -7174     
============================================
- Hits          39353     7908   -31445     
+ Misses        41054     8932   -32122     
+ Partials       3582      857    -2725

see 1732 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

phet
phet reviewed Mar 27, 2023
View reviewed changes
Copy link
Contributor

@phet phet left a comment

Choose a reason for hiding this comment

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

looks ready once finishing a few small details

...ment/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetFinder.java Outdated Show resolved Hide resolved
...ment/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetFinder.java Show resolved Hide resolved
...management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergCatalog.java Outdated Show resolved Hide resolved
...gement/src/main/java/org/apache/gobblin/data/management/copy/iceberg/BaseIcebergCatalog.java Outdated Show resolved Hide resolved
phet
phet approved these changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@phet phet left a comment

Choose a reason for hiding this comment

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

nice work, meeth! looks really good

...management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDataset.java Outdated Show resolved Hide resolved
...ment/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergDatasetFinder.java Outdated Show resolved Hide resolved
...ement/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergRegisterStep.java Outdated Show resolved Hide resolved
...a-management/src/main/java/org/apache/gobblin/data/management/copy/iceberg/IcebergTable.java Outdated Show resolved Hide resolved
@Will-Lo Will-Lo merged commit b428a66 into apache:master Apr 3, 2023
phet added a commit to phet/gobblin that referenced this pull request Apr 12, 2023
@phet
Merge remote-tracking branch 'upstream/master'
4e4f95a 
* upstream/master:
  [GOBBLIN-1807] Replaces conjars.org with conjars.wensel.net (apache#3668)
  [GOBBLIN-1802]Register iceberg table metadata update with destination side catalog (apache#3663)
  Add matching of non-transient exceptions that will avoid failing the container in GMIP (apache#3662)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Will-Lo Will-Lo Will-Lo approved these changes

@phet phet phet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@meethngala @codecov-commenter @phet @Will-Lo