Skip to content

Conversation

@autumnust
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

This PR mostly adds documentation, renaming and fixing typo in the gobblin-icberg module, with extra focus on IcebergMetadataWriter for:

  • Change the way that Transaction object is initialized as it is shared in many different places.
  • Change the way for accessing or initializing AppendFiles and DeleteFiles object. All these changes in TableMetadata object.
  • Adding documentation for places where commit does happen or doesn't happen for reasons.
  • Remove some of the IOException in the method signature wherever they are not required.

Tests

This PR doesn't add or delete any of the functionality so pass all unit tests should be sufficient.

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"

@autumnust
Copy link
Contributor Author

@ZihanLi58 Can you please take a look when you got a chance? Thanks.

Copy link
Contributor

@ZihanLi58 ZihanLi58 left a comment

Choose a reason for hiding this comment

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

+1, LGTM, thanks for the change

@autumnust autumnust merged commit dd20515 into apache:master Aug 6, 2021
jack-moseley pushed a commit to jack-moseley/gobblin that referenced this pull request Aug 24, 2022
apache#3347)

* Add more comments in IcebergMetadataWriter while reading thru the code base

* Fix unit tests post force the compile-time casting of gmce
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants