Skip to content

[HUDI-5634] Rename CDC related classes #7410

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
Merged
xushiyan merged 2 commits into from
Jan 30, 2023
Merged

[HUDI-5634] Rename CDC related classes #7410

xushiyan merged 2 commits into from
Jan 30, 2023

Conversation

@YannByron
Copy link
Contributor

@YannByron YannByron commented Dec 8, 2022

Change Logs

solve some comments left in #6727

Impact

none

Risk level (write none, low medium or high below)

none

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change

  • The config description must be updated if new configs are added or the default value of the configs are changed
  • Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
    ticket number here and follow the instruction to make
    changes to the website.

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@apache apache deleted a comment from YannByron Dec 20, 2022
@apache apache deleted a comment from YannByron Dec 20, 2022
@XuQianJin-Stars
Copy link
Contributor

XuQianJin-Stars commented Dec 20, 2022

@hudi-bot run azure

xushiyan
xushiyan reviewed Jan 28, 2023
View reviewed changes
Copy link
Member

@xushiyan xushiyan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

pls file a jira under epic HUDI-3478 to record the fixes made in this PR. we should make sure fixes are highlighted in the PR and tracked in jira, in where also we should clarify what bug it is.

And pls rebase master since this is very much behind the head

hudi-common/src/main/java/org/apache/hudi/common/table/cdc/HoodieCDCExtractor.java Outdated
cdcFileSplit = new HoodieCDCFileSplit(instantTs, BASE_FILE_DELETE, new ArrayList<>(), Option.empty(), Option.of(beforeFileSlice));
} else if (writeStat.getNumUpdateWrites() == 0L && writeStat.getNumDeletes() == 0
&& writeStat.getNumWrites() == writeStat.getNumInserts()) {
&& writeStat.getNumWrites() > 0) {
Copy link
Member

@xushiyan xushiyan Jan 28, 2023

Choose a reason for hiding this comment

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

can you explain this change pls? this is fixing some corner case?

Copy link
Contributor

@danny0405 danny0405 Jan 28, 2023

Choose a reason for hiding this comment

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

How about rename HoodieCDCInferenceCase into HoodieChangelogType, I'm confused by what Inference Case really means.

Copy link
Contributor Author

@YannByron YannByron Jan 28, 2023

Choose a reason for hiding this comment

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

In my thought, writeStat.getNumWrites() == writeStat.getNumInserts() is right. The change is just for this comment: #6727 (comment). if the case mentioned in this comment, should be fixed in other codes, not here. I will rollback this change.

Copy link
Member

@xushiyan xushiyan Jan 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it's inference case because HoodieCDCExtractor is all about inferring CDC result from commit metadata in different scenarios.

I prefer more concise name over lengthy name that gives the same meaning. anyway are you filing a separate PR for the fixes?

Copy link
Contributor Author

@YannByron YannByron Jan 28, 2023

Choose a reason for hiding this comment

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

this name is followed by #6727 (comment).

Copy link
Contributor

@danny0405 danny0405 Jan 28, 2023

Choose a reason for hiding this comment

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

Alexy is suggesting to use Inference instead because it is a none, but the nameHoodieCDCInferenceCase still confuses me.

hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/cdc/HoodieCDCRDD.scala Outdated
Comment on lines 160 to 157
usesVirtualKeys = false,
usesVirtualKeys = !populateMetaFields,
Copy link
Member

@xushiyan xushiyan Jan 28, 2023

Choose a reason for hiding this comment

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

this is a fix among those renaming changes

Copy link
Contributor

@danny0405 danny0405 Jan 28, 2023

Choose a reason for hiding this comment

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

Seems critical, should be merged for RC2

Copy link
Contributor Author

@YannByron YannByron Jan 28, 2023

Choose a reason for hiding this comment

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

Restore this first. let's keep this pr force on code-improvement.

Copy link
Member

@xushiyan xushiyan Jan 28, 2023

Choose a reason for hiding this comment

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

separate PR for the fix?

Copy link
Member

@xushiyan xushiyan Jan 28, 2023

Choose a reason for hiding this comment

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

#7777

@xushiyan xushiyan requested a review from danny0405 January 28, 2023 03:07
@YannByron YannByron changed the title [HUDI-3478] imporve cdc-related codes [HUDI-5634] imporve cdc-related codes Jan 28, 2023
@xushiyan xushiyan changed the title [HUDI-5634] imporve cdc-related codes [HUDI-5634] Rename CDC related classes Jan 28, 2023
@YannByron
Copy link
Contributor Author

YannByron commented Jan 28, 2023

@hudi-bot run azure

@nsivabalan nsivabalan added the priority:blocker Production down; release blocker label Jan 28, 2023
@apache apache deleted a comment from hudi-bot Jan 29, 2023
@apache apache deleted a comment from YannByron Jan 29, 2023
@apache apache deleted a comment from hudi-bot Jan 29, 2023
@apache apache deleted a comment from YannByron Jan 30, 2023
@apache apache deleted a comment from hudi-bot Jan 30, 2023
@YannByron
Copy link
Contributor Author

YannByron commented Jan 30, 2023

the failed UT is not related to this, will land this.

@hudi-bot
Copy link
Collaborator

hudi-bot commented Jan 30, 2023

CI report:

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

@xushiyan xushiyan merged commit 32f45f0 into apache:master Jan 30, 2023
yihua pushed a commit that referenced this pull request Jan 30, 2023
@YannByron @yihua
[HUDI-5634] Rename CDC related classes (#7410)
b81f49e
fengjian428 pushed a commit to fengjian428/hudi that referenced this pull request Jan 31, 2023
@YannByron
[HUDI-5634] Rename CDC related classes (apache#7410)
9bf5ed2
fengjian428 pushed a commit to fengjian428/hudi that referenced this pull request Apr 5, 2023
@YannByron
[HUDI-5634] Rename CDC related classes (apache#7410)
9251df7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xushiyan xushiyan xushiyan approved these changes

@alexeykudinkin alexeykudinkin alexeykudinkin approved these changes

@danny0405 danny0405 Awaiting requested review from danny0405

Assignees

No one assigned

Labels

priority:blocker Production down; release blocker

Projects

Hudi PR Support
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@YannByron @XuQianJin-Stars @hudi-bot @alexeykudinkin @xushiyan @danny0405 @nsivabalan