Skip to content

Conversation

@liujinhui1994
Copy link
Contributor

@liujinhui1994 liujinhui1994 commented May 17, 2022

Tips

What is the purpose of the pull request

  1. Move clean related configuration to HoodieCleanConfig
  2. Move Archival related configuration to HoodieArchivalConfig
  3. hoodie.compaction.payload.class move this to HoodiePayloadConfig

Committer checklist

  • Has a corresponding JIRA in PR title & commit

  • Commit message is descriptive of the change

  • CI is green

  • Necessary doc changes done or have another open PR

  • For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

@liujinhui1994
Copy link
Contributor Author

@xushiyan please help review... ε(*´・∀・`)з゙

@nsivabalan
Copy link
Contributor

@xushiyan @codope : Can either of you take this up.

@codope codope self-assigned this Jun 24, 2022
@codope codope added priority:blocker Production down; release blocker configs labels Jun 24, 2022
@codope
Copy link
Member

codope commented Jun 24, 2022

Assigned to myself. Will review before end of week.

@liujinhui1994
Copy link
Contributor Author

Please ping me anytime if needed @codope

@xushiyan
Copy link
Member

@liujinhui1994 can you pls rebase and resolve conflicts? also update the PR description to reflect the changes in details, rather than just repeating the jira title. This PR is about moving some clean/archive related configs to its own config classes right?

@liujinhui1994
Copy link
Contributor Author

@xushiyan Yes, you are right.
I will follow your request

@hudi-bot
Copy link
Collaborator

CI report:

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

@liujinhui1994
Copy link
Contributor Author

@codope @xushiyan please review

@codope
Copy link
Member

codope commented Jul 5, 2022

@liujinhui1994 I'll pick up the review this week. A couple of high-level questions:

  1. Are there any default behaviour changes? If yes, then I would suggest to separate out the default value changes to a separate PR.
  2. For the renames, is the backward compatibility handled? If not, you can explore the usage of ConfigProperty#withAlternatives API. Let's add some compatibility UTs if not already added.

Copy link
Member

@codope codope left a comment

Choose a reason for hiding this comment

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

@liujinhui1994 The changes look clean. It would be helpful for configurations page on hudi website as well.
The summary of this diff is mainly that the cleaning, archival, payload related configs have been extracted away from HoodieCompactionConfig. I did not spot any config key changes so we should be good in terms of backward compatibility in this case.
But, have we thought about users who use contants? For e.g.

df.write.format("hudi")
  ...
  .option(HoodieCompactionConfig.AUTO_CLEAN.key, "true")
  ...

Won't this patch affect such users?

@liujinhui1994
Copy link
Contributor Author

I understand your idea. In very strict cases, the user will be required to modify the specified class. I understand that the impact of this is not big. After upgrading, users need to convert HoodieCompactionConfig to HoodieCleanConfig/HoodiePayloadConfig.

Or it won't matter if the user uses the following:
df.write.format("hudi")
...
.option("hoodie.archive.automatic", "true")

@codope

@liujinhui1994
Copy link
Contributor Author

My mistake Force push. cry. There seems to be no recovery. I redevelop some @codope

@liujinhui1994
Copy link
Contributor Author

#6061 Sorry, to go here @codope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:blocker Production down; release blocker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants