Repository navigation
fix(core): refuse to downgrade a table ordered on several fields below version 9 - #20348
wombatu-kun wants to merge 2 commits into
Conversation
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This makes a downgrade below version 9 fail early, before any rollback, compaction or metadata table change, when the table orders records on several fields, and it now writes the parsed single field to the precombine key. 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. One small comment-clarity suggestion below; otherwise the code reads cleanly.
cc @yihua
| propertiesToAdd.put(HoodieTableConfig.PRECOMBINE_FIELD, orderingFields); | ||
| List<String> orderingFields = getOrderingFieldsBelowVersionNine(tableConfig); | ||
| if (orderingFields.size() == 1) { | ||
| // the parsed field, so that a stray separator as in `ts,` does not end up in a key read as one field name |
There was a problem hiding this comment.
🤖 nit: could you reword this comment as a full sentence, e.g. "Write the parsed field rather than the raw string, so a stray separator as in ts, does not end up in the key"? As written it is a fragment that is hard to follow.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20348 +/- ##
============================================
+ Coverage 80.73% 80.74% +0.01%
- Complexity 35169 35319 +150
============================================
Files 2553 2562 +9
Lines 143582 144150 +568
Branches 17486 17609 +123
============================================
+ Hits 115920 116400 +480
- Misses 19701 19777 +76
- Partials 7961 7973 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| SupportsUpgradeDowngrade upgradeDowngradeHelper) { | ||
| final HoodieTable table = upgradeDowngradeHelper.getTable(config, context); | ||
| HoodieTableMetaClient metaClient = table.getMetaClient(); | ||
| validateOrderingFields(metaClient.getTableConfig(), HoodieTableVersion.EIGHT); |
There was a problem hiding this comment.
nit: UpgradeDowngrade.run already validates this with the real target version before any handler runs, so this call only fires when the handler is driven directly, and it hardcodes 8 even for a 10 to 6 request.
There was a problem hiding this comment.
Done aef75ce. Only UpgradeDowngrade.run checks now, so a 10 to 6 request reports version 6, which TestUpgradeDowngradeOrchestration now covers.
| when(tableConfig.getOrderingFieldsStr()).thenReturn(Option.of("ts,seq")); | ||
| HoodieUpgradeDowngradeException exception = assertThrows(HoodieUpgradeDowngradeException.class, | ||
| () -> handler.downgrade(config, context, "anyInstant", upgradeDowngradeHelper)); | ||
| assertEquals("Cannot downgrade to table version 8: hoodie.table.ordering.fields=ts,seq orders records on several fields, " |
There was a problem hiding this comment.
nit: three tests across three modules assert this message verbatim, so any wording tweak breaks all of them - matching on the ordering.fields part like the orchestration test does would be enough.
There was a problem hiding this comment.
Done aef75ce. No test asserts the whole message now: this test went away with the handler check, and TestUpgradeDowngrade matches the same leading part as the orchestration test, target version included.
…owngrade.run and match the message prefix in tests
This branch has not been deployed
4 participants
Describe the issue this Pull Request addresses
Closes #20347
The 9 to 8 downgrade copied several ordering fields as they are into
hoodie.table.precombine.field, which Hudi 1.0.x and 0.x read as a single field name, so the release it targets could not upsert into the downgraded table.Summary and Changelog
UpgradeDowngrade.runrefuses a downgrade from version 9 or above to below 9 of a table ordered on several fields, before the rollback, the compaction and the metadata table downgrade. The message names the fields, says why 1.0.x and 0.x cannot read them, and points to keeping the table at version 9 or recreating it with a single ordering field.NineToEightDowngradeHandlerholds the check, which onlyUpgradeDowngrade.runcalls. The check uses the fields the downgrade would write, soMySqlDebeziumAvroPayloadtables still go back to their single_event_seq, and the handler now writes the parsed field, so a stray separator as ints,no longer lands in the legacy key.TestNineToEightDowngradeHandlercovers a single field,ts,and the MySQL Debezium mapping.TestUpgradeDowngradeOrchestrationrefuses 10 to 8 and 10 to 6 downgrades before any step and lets 10 to 9 with two fields and 10 to 8 for MySQL Debezium proceed.TestUpgradeDowngraderefuses the version 9 MOR fixture ordered onts,nameat versions 8 and 6 and checks that the data and metadata tables keep their version, timeline, ordering fields and log files.Impact
A downgrade below version 9 of a table ordered on several fields now fails with
Cannot downgrade to table version 8: hoodie.table.ordering.fields=ts,name orders records on several fields, which Hudi 1.0.x and 0.x cannot read. ...and leaves the table as it was. Before, it reported success and lefthoodie.table.precombine.field=ts,name, a field that 1.0.2 upserts and MOR merges look for and do not find. Every other downgrade behaves as before. Thedowngrade_tableprocedure turns the failure into afalseresult and logs the message.Risk Level
low
The check only rejects downgrades whose result was unusable, and it runs before anything changes. Verified: the refusal tests fail on master with
Expected org.apache.hudi.exception.HoodieUpgradeDowngradeException to be thrown, but nothing was thrown., and the downgrade on master writeshoodie.table.precombine.field=ts,name, which Hudi 1.0.2 and 0.15.0 resolve as their precombine field.Documentation Update
none
Contributor's checklist