-
Notifications
You must be signed in to change notification settings - Fork 2.5k
refactor: Add Lombok Builder annotation to HoodieLogFormat #17785
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.
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
base: master
Are you sure you want to change the base?
refactor: Add Lombok Builder annotation to HoodieLogFormat #17785
Conversation
d21169c to
fee094e
Compare
fee094e to
342a043
Compare
342a043 to
857cb70
Compare
CTTY
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @voonhous , thanks for the PR and it mostly looks good! I've left some comments
| ) throws IOException { | ||
| super(bufferSize, storage, parentPath, logFileId, fileExtension, instantTime, logVersion, logWriteToken, | ||
| suffix, fileLen, sizeThreshold, fileCreationCallback, tableVersion); | ||
| this.outputStream = outputStream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right, outputStream should be initialized lazily rather than set like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch. Removed this and added a comment.
| protected Integer logVersion; | ||
| // file len of this log file | ||
| private Long fileLen = 0L; | ||
| protected Long fileLen = 0L; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use fileSize not fileLen since most APIs like HoodieLogFile still refer to it as getFileSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, we should standardise the naming. In the other hudi-common Lombok refactoring, i've changed them to getFileSize too, so this makes sense.
Describe the issue this Pull Request addresses
The current implementation of
HoodieLogFormat.WriterBuilderis convoluted:HoodieLogFormatinterface.ReflectionUtilsto load the default writer implementation via a String class name, which is brittle and bypasses compile-time checks, a change introduced in #11207 to decouplehudi-commonwith hadoop dependenciesThis PR refactors the log writer to use Lombok's
@Builder, standardizing the fluent API and improving type safety across the codebase.Summary and Changelog
This refactor simplifies the construction of
HoodieLogFormatwriters by leveraging Lombok and improving the class hierarchy.Key Changes:
HoodieLogFormat.Writerfrom an interface to an abstract class to centralize shared fields and construction logic.Writerbase constructor. This ensures that all writer implementations follow the same versioning and path-generation logic.@Builderto theHoodieLogFormatWriterconstructor, replacing the manualWriterBuilder.withprefix).TestHoodieLogWriterBuildertoTestHoodieLogFormatWriterBuilderto reflect the new structure.Impact
onParentPathis nowwithParentPath).HoodieLogFormat.Risk Level
Low. This is a structural refactor. The core logic for log writing and versioning remains unchanged, just relocated to the base class constructor.
Documentation Update
None
Contributor's checklist