feat(common): add hoodie.table.version.pinned ceiling guard - #20009
shangxinli wants to merge 1 commit into
Conversation
Adds an opt-in ceiling on hoodie.table.version so an accidental upgrade is caught before it silently makes a table unreadable by older readers or blocks a rollback. Loading or writing a table whose version exceeds the pin throws HoodieTableVersionPinExceededException and increments a per-table counter; a version at or below the pin is unaffected. The pin can be set per table in hoodie.properties, or as a JVM system property of the same key to apply it fleet-wide without editing every table. The table value takes precedence, and a blank table-level value falls through to the system property rather than silently shadowing it. The default UN_PINNED disables the check, so there is no behavior change unless it is set. Enforcement lives in a single helper invoked from three points: - HoodieTableConfig.getTableVersion(HoodieConfig), covering read paths - HoodieTableConfig.setTableVersion, the backstop for every writer, including new-table creation via HoodieTableMetaClient.TableBuilder - UpgradeDowngrade.run, which validates the target version up front so it fails before any rollback or compaction work is attempted A pin violation raised by the nested metadata table upgrade is rethrown as-is rather than wrapped, since it is a configuration decision to honor and not a metadata table failure that disabling the metadata table would resolve. The counter is emitted through org.apache.hudi.common.metrics.Registry, so any configured MetricsReporter picks it up without extra wiring; emission failures are swallowed and can never mask the thrown exception.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #20009 +/- ##
============================================
+ Coverage 80.34% 80.35% +0.01%
- Complexity 34748 34765 +17
============================================
Files 2545 2546 +1
Lines 142493 142539 +46
Branches 17312 17317 +5
============================================
+ Hits 114489 114544 +55
+ Misses 20107 20098 -9
Partials 7897 7897
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Since 1.0.0, Hudi introduced a write option |
| public static final ConfigProperty<String> MAX_ALLOWED_TABLE_VERSION = ConfigProperty | ||
| .key("hoodie.table.version.pinned") | ||
| .defaultValue("UN_PINNED") | ||
| .withDocumentation("Ceiling on hoodie.table.version: loading or writing a table whose version exceeds " |
There was a problem hiding this comment.
loading or writing a table whose version exceeds this value throws immediately
Instead of upgrade first then validate and throw, I would suggest we disable the upgrade in the very first place which is more clear and also user-friendly.
There was a problem hiding this comment.
🤖 +1, and there's a concrete reason to scope this to the upgrade path only: UpgradeDowngrade.run() calls getTableVersion() to get fromVersion, and that now throws whenever the current version is above the pin. So a table that has already slipped past the pin can't be downgraded back — which is the rollback scenario the PR description says it wants to protect. Gating only needsUpgrade/the upgrade branch (and leaving getTableVersion alone) would avoid that.
| .withDocumentation("Ceiling on hoodie.table.version: loading or writing a table whose version exceeds " | ||
| + "this value throws immediately, so an accidental upgrade is caught before it silently makes the " | ||
| + "table unreadable by older readers. A version at or below the pin is unaffected. Also honored as " | ||
| + "a JVM system property of the same key (this config value takes precedence over the system " |
There was a problem hiding this comment.
a JVM system property of the same key (this config value takes precedence over the system
looks risky and might cause config leaks.
There was a problem hiding this comment.
🤖 Agree this is worth reconsidering. The system property is consulted on every getTableVersion() call inside hudi-common, so one long-lived JVM (shared Spark driver, Flink JM, timeline server) with the property set would apply the pin to every table it touches, including reads of unrelated tables above the pin. It also inverts the usual precedence here since any table's hoodie.properties can set UN_PINNED to opt out of the fleet-wide pin. A write-config option flowing through HoodieWriteConfig would fit the existing pattern better.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR adds an opt-in hoodie.table.version.pinned ceiling enforced in HoodieTableConfig.getTableVersion/setTableVersion and up front in UpgradeDowngrade.run, with a new exception type and a per-table counter. A few things worth double-checking in the inline comments: the fail-fast guarantee doesn't cover the eager rollback that BaseHoodieWriteClient.tryUpgrade performs before run is entered, a pin value above this binary's newest known version currently makes the table unloadable rather than being trivially satisfied, and the counter is only consumed by Metrics.flush()/shutdown() so it may never reach a reporter. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. A few naming/consistency nits below (mostly around the "pin" terminology), otherwise the change is well-structured and tested.
| public void run(HoodieTableVersion toVersion, String instantTime) { | ||
| // Fail fast, before any rollback/compaction work is attempted, if the target version exceeds the | ||
| // configured table version pin. | ||
| metaClient.getTableConfig().validateVersionPin(toVersion); |
There was a problem hiding this comment.
🤖 This does fail before the rollback/compaction inside run, but the main writer entry point BaseHoodieWriteClient.tryUpgrade (L1690–1702) calls needsUpgrade and then eagerly rolls back all failed/inflight writes before it ever reaches run, so a pinned table with a higher hoodie.write.table.version will still get those rollbacks and only then hit the pin. Would it make sense to also check the pin in needsUpgrade/needsUpgradeOrDowngrade (or at the top of tryUpgrade) so the writer really does no work first?
| throw new NumberFormatException("Table version pin must not be negative: " + versionCode); | ||
| } | ||
| // Validates that the code corresponds to a recognized table version. | ||
| HoodieTableVersion.fromVersionCode(versionCode); |
There was a problem hiding this comment.
🤖 If a fleet-wide pin (system property) is set to a version code that this particular binary doesn't know yet (e.g. pin=11 while an older reader in the same fleet only knows up to 10), this throws and the table becomes unloadable for that reader, even though its actual version is fine. Since a pin above the newest version this binary knows can never be exceeded, would it be safer to treat that case as satisfied (or clamp to the max known version) rather than fail?
| */ | ||
| private static void emitVersionPinExceededMetric(String tableName) { | ||
| try { | ||
| Registry.getRegistryOfClass(tableName, "hoodie.table.version.pin", LocalRegistry.class.getName()) |
There was a problem hiding this comment.
🤖 The description says any configured MetricsReporter picks this up, but the only consumers of Registry.getAllMetrics are Metrics.flush()/shutdown(), which exist only when a write client has metrics enabled and run at commit/JVM-exit. On the read path (or a failed write that never reaches a flush) the counter just accumulates in the static REGISTRY_MAP. Is that the intended visibility, or should this be documented as best-effort?
| + " to identify what upgrade/downgrade paths happened on the table. This is only configured " | ||
| + "when the table is initially setup."); | ||
|
|
||
| public static final ConfigProperty<String> MAX_ALLOWED_TABLE_VERSION = ConfigProperty |
There was a problem hiding this comment.
🤖 nit: MAX_ALLOWED_TABLE_VERSION doesn't match the config key hoodie.table.version.pinned — a reader searching for the pin field by name would look for something like TABLE_VERSION_PIN first. Consider renaming the constant to match the key/concept used elsewhere (e.g. TABLE_VERSION_PINNED).
| Option<Integer> pinnedVersionCode = resolvePinnedVersionCode(config); | ||
| if (!pinnedVersionCode.isPresent() || version.versionCode() <= pinnedVersionCode.get()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🤖 nit: resolvePinnedVersionCode mixes resolution, validation, and error-throwing for malformed values in one method — could you split parsing/validation into a small helper so the happy-path resolution logic reads more linearly?
| public static final ConfigProperty<String> MAX_ALLOWED_TABLE_VERSION = ConfigProperty | ||
| .key("hoodie.table.version.pinned") | ||
| .defaultValue("UN_PINNED") | ||
| .withDocumentation("Ceiling on hoodie.table.version: loading or writing a table whose version exceeds " |
There was a problem hiding this comment.
🤖 +1, and there's a concrete reason to scope this to the upgrade path only: UpgradeDowngrade.run() calls getTableVersion() to get fromVersion, and that now throws whenever the current version is above the pin. So a table that has already slipped past the pin can't be downgraded back — which is the rollback scenario the PR description says it wants to protect. Gating only needsUpgrade/the upgrade branch (and leaving getTableVersion alone) would avoid that.
| .withDocumentation("Ceiling on hoodie.table.version: loading or writing a table whose version exceeds " | ||
| + "this value throws immediately, so an accidental upgrade is caught before it silently makes the " | ||
| + "table unreadable by older readers. A version at or below the pin is unaffected. Also honored as " | ||
| + "a JVM system property of the same key (this config value takes precedence over the system " |
There was a problem hiding this comment.
🤖 Agree this is worth reconsidering. The system property is consulted on every getTableVersion() call inside hudi-common, so one long-lived JVM (shared Spark driver, Flink JM, timeline server) with the property set would apply the pin to every table it touches, including reads of unrelated tables above the pin. It also inverts the usual precedence here since any table's hoodie.properties can set UN_PINNED to opt out of the fleet-wide pin. A write-config option flowing through HoodieWriteConfig would fit the existing pattern better.
5 participants
Describe the issue this Pull Request addresses
There is no way to stop a table from being upgraded past a known-good table version. During a staged 0.14 -> 1.x rollout an accidental auto-upgrade can silently make a table unreadable by older readers, or block a rollback, and it is usually only noticed later when a reader fails.
Summary and Changelog
Adds an opt-in ceiling on
hoodie.table.version. Exceeding the pin throwsHoodieTableVersionPinExceededExceptionand increments a per-table counter; at or below the pin is unaffected. DefaultUN_PINNEDdisables the check.hoodie.table.version.pinned, settable per table inhoodie.propertiesor as a JVM system property of the same key for a fleet-wide pin. The table value wins; a blank one falls through instead of shadowing it.HoodieTableConfig.getTableVersion(reads),HoodieTableConfig.setTableVersion(backstop for all writers, including table creation), andUpgradeDowngrade.run, which checks up front so it fails before any rollback or compaction.common.metrics.Registry, so any configuredMetricsReporterpicks it up. Emission failures are swallowed and cannot mask the exception.Impact
None by default —
UN_PINNEDleaves all existing paths unchanged. When set, reads and writes above the pin fail fast by design.Risk Level
low
Gated behind a config that defaults to disabled. 19 new tests cover ceiling semantics, disabled paths, system-property precedence, malformed/negative/blank values, pinning an existing table, and metric emission. Full
hudi-common,hudi-client-commonandhudi-hadoop-commonsuites pass unchanged.Documentation Update
The new config carries a full
withDocumentationdescription and is picked up by the generated config docs. No other website change needed.Contributor's checklist