feat(flink): add RocksDB partitioned RLI Cache Flink Options - #19641
feat(flink): add RocksDB partitioned RLI Cache Flink Options#19641HuangZhenQiu wants to merge 1 commit into
Conversation
7811c79 to
4cdd217
Compare
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR exposes the seven RFC-107 RocksDB partitioned RLI cache configs as ConfigOptions in FlinkOptions, ahead of the actual implementation. Since these are declarations only with no wiring yet, there's little functional surface to exercise. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. A few small naming and comment nits below.
cc @yihua
|
|
||
| @AdvancedConfig | ||
| public static final ConfigOption<Boolean> INDEX_RLI_ROCKSDB_CACHE_INVALIDATE_ON_REPLACE_COMMIT = ConfigOptions | ||
| .key("hoodie.record.index.cache.rocksdb.invalidate.on.replacecommit") |
There was a problem hiding this comment.
🤖 nit: replacecommit runs two words together without a separator, which breaks the standard hoodie.xxx.yyy.zzz dot-separated convention. Could you use replace.commit instead?
There was a problem hiding this comment.
🤖 Worth noting that replacecommit isn't an arbitrary run-together — it mirrors Hudi's own action name, HoodieTimeline.REPLACE_COMMIT_ACTION = "replacecommit" (hudi-common .../HoodieTimeline.java). Since this config keys off that action, keeping it as one token stays consistent with how the action is spelled everywhere else in the timeline; replace.commit would actually diverge from it.
There was a problem hiding this comment.
🤖 One thing worth factoring in here: this key derives from the timeline action name, which is spelled as a single token — HoodieTimeline.REPLACE_COMMIT_ACTION = "replacecommit" (hudi-common/.../HoodieTimeline.java:64). Splitting it into replace.commit would actually diverge from how the action is written everywhere in the timeline, so replacecommit may be the more consistent choice despite looking run-together.
| .withDescription("Local directory for RocksDB partitioned record level index cache data."); | ||
|
|
||
| @AdvancedConfig | ||
| public static final ConfigOption<Integer> INDEX_RLI_ROCKSDB_CACHE_BOOTSTRAP_DAYS = ConfigOptions |
There was a problem hiding this comment.
🤖 nit: "bootstrap" is already a well-known term in Hudi (the Bootstrap feature for ingesting external tables). Using BOOTSTRAP_DAYS here for a pre-loading / warm-up window might confuse readers — have you considered PRELOAD_DAYS or WARMUP_DAYS?
| public static final ConfigOption<Long> INDEX_RLI_ROCKSDB_CACHE_PARTITION_TTL_HOURS = ConfigOptions | ||
| .key("hoodie.record.index.cache.rocksdb.partition.ttl.hours") | ||
| .longType() | ||
| .defaultValue(168L) // default 7 days |
There was a problem hiding this comment.
🤖 nit: the inline // default 7 days comment duplicates what the withDescription already says — could you drop it to reduce noise?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19641 +/- ##
=========================================
Coverage 77.70% 77.71%
+ Complexity 32988 32983 -5
=========================================
Files 2524 2524
Lines 139525 139560 +35
Branches 16786 16786
=========================================
+ Hits 108424 108464 +40
+ Misses 23475 23466 -9
- Partials 7626 7630 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
4cdd217 to
1948154
Compare
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds seven RFC-107 RocksDB partitioned RLI cache configs as ConfigOptions in Flink's FlinkOptions; they're declarations only, with no consuming logic yet. No functional correctness or architectural issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review. A couple of small naming and comment nits below.
cc @yihua
|
|
||
| @AdvancedConfig | ||
| public static final ConfigOption<Boolean> INDEX_RLI_ROCKSDB_CACHE_INVALIDATE_ON_REPLACE_COMMIT = ConfigOptions | ||
| .key("hoodie.record.index.cache.rocksdb.invalidate.on.replacecommit") |
There was a problem hiding this comment.
🤖 nit: replacecommit is missing a dot separator — could you rename this to hoodie.record.index.cache.rocksdb.invalidate.on.replace.commit to stay consistent with the dot-separated key convention used everywhere else?
There was a problem hiding this comment.
🤖 Before renaming to ...invalidate.on.replace.commit, note that this token mirrors the timeline action name, which is a single word: HoodieTimeline.REPLACE_COMMIT_ACTION = "replacecommit" (hudi-common/.../HoodieTimeline.java:64). Dotting it would diverge from the action spelling used across the timeline, so replacecommit is arguably the consistent form here.
| public static final ConfigOption<Long> INDEX_RLI_ROCKSDB_CACHE_PARTITION_TTL_HOURS = ConfigOptions | ||
| .key("hoodie.record.index.cache.rocksdb.partition.ttl.hours") | ||
| .longType() | ||
| .defaultValue(168L) // default 7 days |
There was a problem hiding this comment.
🤖 nit: the // default 7 days inline comment is already covered by the description string — could you drop it to avoid duplication that can drift if the default changes?
|
|
||
| @AdvancedConfig | ||
| public static final ConfigOption<Boolean> INDEX_RLI_ROCKSDB_CACHE_INVALIDATE_ON_REPLACE_COMMIT = ConfigOptions | ||
| .key("hoodie.record.index.cache.rocksdb.invalidate.on.replacecommit") |
There was a problem hiding this comment.
🤖 Worth noting that replacecommit isn't an arbitrary run-together — it mirrors Hudi's own action name, HoodieTimeline.REPLACE_COMMIT_ACTION = "replacecommit" (hudi-common .../HoodieTimeline.java). Since this config keys off that action, keeping it as one token stays consistent with how the action is spelled everywhere else in the timeline; replace.commit would actually diverge from it.
1948154 to
b32df4f
Compare
Exposes the 7 configuration properties defined by RFC-107 (Support data partition aware RocksDB RecordIndexBackend) in FlinkOptions
b32df4f to
d2df072
Compare
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the RFC follow-up! This exposes the seven RFC-107 RocksDB partitioned RLI cache properties as Flink ConfigOptions and aligns the RFC's config table with the index.rli.* keys. No correctness issues stand out at this stage; the remaining naming/convention points raised in earlier rounds can be settled, and a Hudi committer or PMC member can take it from here for a deeper design review.
cc @yihua
|
|
||
| @AdvancedConfig | ||
| public static final ConfigOption<Boolean> INDEX_RLI_ROCKSDB_CACHE_INVALIDATE_ON_REPLACE_COMMIT = ConfigOptions | ||
| .key("hoodie.record.index.cache.rocksdb.invalidate.on.replacecommit") |
There was a problem hiding this comment.
🤖 One thing worth factoring in here: this key derives from the timeline action name, which is spelled as a single token — HoodieTimeline.REPLACE_COMMIT_ACTION = "replacecommit" (hudi-common/.../HoodieTimeline.java:64). Splitting it into replace.commit would actually diverge from how the action is written everywhere in the timeline, so replacecommit may be the more consistent choice despite looking run-together.
|
|
||
| @AdvancedConfig | ||
| public static final ConfigOption<Boolean> INDEX_RLI_ROCKSDB_CACHE_INVALIDATE_ON_REPLACE_COMMIT = ConfigOptions | ||
| .key("hoodie.record.index.cache.rocksdb.invalidate.on.replacecommit") |
There was a problem hiding this comment.
🤖 Before renaming to ...invalidate.on.replace.commit, note that this token mirrors the timeline action name, which is a single word: HoodieTimeline.REPLACE_COMMIT_ACTION = "replacecommit" (hudi-common/.../HoodieTimeline.java:64). Dotting it would diverge from the action spelling used across the timeline, so replacecommit is arguably the consistent form here.
Exposes the 7 configuration properties defined by RFC-107 (Support data partition aware RocksDB RecordIndexBackend) as ConfigOptions in FlinkOptions
Describe the issue this Pull Request addresses
Add new configs for RFC-107 for follow up implementation.
Summary and Changelog
7 new configs are added into FlinkOptions.
Impact
none
Risk Level
none
Documentation Update
These config will be added into documents once RFC-107 is fully implemented.
Contributor's checklist