fix(flink): support native log max file size config - #19630
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19630 +/- ##
============================================
+ Coverage 77.49% 77.54% +0.05%
- Complexity 32799 32930 +131
============================================
Files 2522 2524 +2
Lines 139179 139513 +334
Branches 16734 16865 +131
============================================
+ Hits 107855 108190 +335
+ Misses 23748 23738 -10
- Partials 7576 7585 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
-1, I don't think we should use the |
cshuo
changed the title
ca1e491 to
062606c
Compare
Fixed. Following the suggestion, added hoodie.native.log.max.file.size with a file-format-specific fallback. |
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds a common hoodie.native.log.max.file.size config and wires it into the Flink native Parquet log writer, falling back to hoodie.parquet.max.file.size when unset while leaving base Parquet files unaffected. I traced the path (path-based getFileWriter → newParquetFileWriter(instantTime, storagePath, ...) → parquet writer canWrite()) and the plumbing looks correct, including the contains() guard before getLong and the base-vs-native-log discrimination. One cross-engine scoping question worth double-checking in the inline comment. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. Code looks clean overall — one minor naming observation below.
| .withDocumentation("Target size in bytes for parquet files produced by Hudi write phases. " | ||
| + "For DFS, this needs to be aligned with the underlying filesystem block size for optimal performance."); | ||
|
|
||
| public static final ConfigProperty<String> NATIVE_LOG_MAX_FILE_SIZE = ConfigProperty |
There was a problem hiding this comment.
🤖 This config lives in hudi-common and the docs read as format/engine-generic, but only the Flink HoodieRowDataFileWriterFactory honors it — HoodieSparkFileWriterFactory.newParquetFileWriter still uses PARQUET_MAX_FILE_SIZE unconditionally for native logs. Is the Flink-only scope intentional? A Spark user setting hoodie.native.log.max.file.size would see it silently ignored; might be worth narrowing the doc or noting the limitation.
| HoodieConfig config, HoodieRowDataParquetWriteSupport writeSupport, StoragePath storagePath) { | ||
| // Native logs handled by this factory are write-once Parquet files whose rollover is decided by | ||
| // the Parquet writer's canWrite(), not by LOGFILE_MAX_SIZE used for Hudi log containers. Preserve | ||
| // the Parquet target as the fallback unless a dedicated native-log target is explicitly configured. |
There was a problem hiding this comment.
🤖 nit: with three overloads all named getParquetConfig, the StoragePath variant's intent ("pick the right max size based on path") isn't obvious at a glance — you have to read each signature to follow the dispatch chain. Have you considered inlining the maxFileSize resolution directly in newParquetFileWriter (a couple of lines before the existing call), and keeping just the single long-taking overload? That would make the decision visible at the call site without the indirection.
5 participants
Describe the issue this Pull Request addresses
Closes #19629.
Flink native Parquet log writers currently use
hoodie.parquet.max.file.sizeas their rollover threshold, so native log files cannot be sized independently from base Parquet files.Summary and Changelog
hoodie.native.log.max.file.sizeconfiguration in bytes.hoodie.parquet.max.file.sizewhen it is not configured, preserving the current default behavior.Impact
This is backward compatible by default. Users can opt in to a different native log rollover threshold without changing the target size of base Parquet files or reusing the inline log-file size setting.
Risk Level
Low. The new configuration is opt-in, and the existing Parquet target remains the fallback.
Documentation Update
The new common configuration is documented in
HoodieStorageConfig.Contributor's checklist
Validation:
mvn -pl hudi-client/hudi-flink-client -am -Dtest=TestHoodieRowDataParquetConfigInjector#testNativeLogMaxFileSize -Dsurefire.failIfNoSpecifiedTests=false -DskipITs -DskipSparkTests -DskipScalaTests test(1 test, 0 failures, 0 errors).