Skip to content

[HUDI-2150] Rename/Restructure configs for better modularity#5608

Closed
liujinhui1994 wants to merge 0 commit intoapache:masterfrom
liujinhui1994:HUDI-2150
Closed

[HUDI-2150] Rename/Restructure configs for better modularity#5608
liujinhui1994 wants to merge 0 commit intoapache:masterfrom
liujinhui1994:HUDI-2150

Conversation

@liujinhui1994
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@nsivabalan
Copy link
Copy Markdown
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
Copy Markdown
Member

codope commented Jun 24, 2022

Assigned to myself. Will review before end of week.

@liujinhui1994
Copy link
Copy Markdown
Contributor Author

Please ping me anytime if needed @codope

@xushiyan
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@hudi-bot
Copy link
Copy Markdown
Collaborator

CI report:

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

@liujinhui1994
Copy link
Copy Markdown
Contributor Author

@codope @xushiyan please review

@codope
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@liujinhui1994
Copy link
Copy Markdown
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