yihua commented on code in PR #20063:
URL: https://github.com/apache/hudi/pull/20063#discussion_r4110081607
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/UpgradeDowngrade.java:
##########
@@ -422,13 +422,21 @@ private void
performRollbackAndCompactionIfRequired(HoodieTableVersion fromVersi
* does not carry {@link HoodieTableConfig#COMPLEX_KEYGEN_ENCODING} yet, and
persists it with the version change.
* The data has to be read up-front: the 7 to 8 hop rewrites the timeline on
storage while
* {@code hoodie.properties} still reports the old version.
+ *
+ * <p>The data is read through a meta client that takes the timeline layout
from {@code hoodie.properties}:
+ * callers such as the upgrade procedure and hudi-cli pin the layout of the
write config, which is newer than
+ * the layout a table below version 8 has on storage, so their meta client
cannot load its timeline.
*/
private void resolveComplexKeygenEncoding(Map<ConfigProperty, String>
tablePropsToAdd, String operation) {
HoodieTableConfig tableConfig = metaClient.getTableConfig();
if (!KeyGenUtils.requireComplexKeyGenEncodingTracked(tableConfig) ||
tableConfig.getComplexKeyGenEncoding().isPresent()) {
return;
}
- ComplexKeyGenEncoding encoding =
KeyGenUtils.resolveComplexKeyGenEncodingForWrite(metaClient, config)
+ HoodieTableMetaClient dataMetaClient = HoodieTableMetaClient.builder()
+ .setStorage(metaClient.getStorage())
+ .setBasePath(metaClient.getBasePath())
+ .build();
+ ComplexKeyGenEncoding encoding =
KeyGenUtils.resolveComplexKeyGenEncodingForWrite(dataMetaClient, config)
Review Comment:
One case I'm unsure about: if a previous upgrade died after the 7 to 8 hop
had moved the instants into `.hoodie/timeline` (hoodie.properties is only
rewritten at the end of `run()`), a retry reads layout 1 here, finds an empty
commit timeline, and `deduceComplexKeyGenEncodingFromData` records
`FIELD_PREFIXED` even for a bare-key table. The write path from #19304 has the
same exposure, so it may belong in a follow-up, but would it make sense to
treat an empty timeline alongside an existing V2 timeline directory as "cannot
deduce" rather than "never written"?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/UpgradeDowngrade.java:
##########
@@ -422,13 +422,21 @@ private void
performRollbackAndCompactionIfRequired(HoodieTableVersion fromVersi
* does not carry {@link HoodieTableConfig#COMPLEX_KEYGEN_ENCODING} yet, and
persists it with the version change.
* The data has to be read up-front: the 7 to 8 hop rewrites the timeline on
storage while
* {@code hoodie.properties} still reports the old version.
+ *
+ * <p>The data is read through a meta client that takes the timeline layout
from {@code hoodie.properties}:
+ * callers such as the upgrade procedure and hudi-cli pin the layout of the
write config, which is newer than
+ * the layout a table below version 8 has on storage, so their meta client
cannot load its timeline.
*/
private void resolveComplexKeygenEncoding(Map<ConfigProperty, String>
tablePropsToAdd, String operation) {
HoodieTableConfig tableConfig = metaClient.getTableConfig();
if (!KeyGenUtils.requireComplexKeyGenEncodingTracked(tableConfig) ||
tableConfig.getComplexKeyGenEncoding().isPresent()) {
return;
}
- ComplexKeyGenEncoding encoding =
KeyGenUtils.resolveComplexKeyGenEncodingForWrite(metaClient, config)
+ HoodieTableMetaClient dataMetaClient = HoodieTableMetaClient.builder()
Review Comment:
Rather than building a second meta client here, could we drop the
`setLayoutVersion` pin in `UpgradeOrDowngradeProcedure` and `SparkMain`
instead? That pin is the actual bug: since the 7 to 8 hop took over the layout
migration, it only lets the caller's meta client disagree with
`hoodie.properties`, and `run()` otherwise touches just the table config and
storage, so removing it changes nothing for v8+ tables.
For the regression test, could it go through `call upgrade_table` in
`TestUpgradeOrDowngradeProcedure` on the v6 complex keygen fixtures (both the
`field:value` and the bare-key one), asserting the table version, the recorded
encoding, and that a follow-up upsert of existing keys updates them in place
with no duplicates? The existing procedure tests only upgrade tables they first
downgraded from the current version, where the encoding is already recorded,
which is how this slipped through.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
