[HUDI-6397][HUDI-6759] Fixing misc bugs w/ metadata table #9546

nsivabalan · 2023-08-26T18:56:21Z

Change Logs

Disabling entire MDT after enabling needs some fixes.
We do have some special instants times like initialization instant times, compaction, clean etc in MDT timeline. During InstantRange check, these should be considered as valid.

Impact

allows users to disable metadata using write configs cleanly.
Valid instants consideration while reading from MDT is solid now. We are going to treat any special instant time (that has additional suffix compared to DT's commit time) as valid.

Especially with MDT partition initialization, the suffix is dynamic, and so we can't really find exact match. So, might have to go with total instant time length and treat all special instant times as valid ones.

In the LogRecordReader, we will first ignore any uncommitted instants. And then if its completed in MDT timeline, we check w/ the instantRange. and so its should be fine to return true for any special instant times.

Risk level (write none, low medium or high below)

medium

Documentation Update

N/A

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/table/HoodieSparkTable.java

hudi-common/src/main/java/org/apache/hudi/common/table/log/InstantRange.java

yihua · 2023-09-02T00:27:48Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java

    return !HoodieTableMetadata.isMetadataTable(metaClient.getBasePath())
-        && !config.isMetadataTableEnabled()
-        && !metaClient.getTableConfig().getMetadataPartitions().isEmpty();


Why do we check condition (3) before?

yeah. When we upstreamed patches from uber, we had an idea that we will not allow disabling metadata table via write configs only. Once enabled, user has to go via hudi-cli to disable it.
but due to operational overhead, we are reverting this change.

yihua · 2023-09-02T00:30:03Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

@@ -153,6 +153,8 @@ static boolean isValidSuffix(String suffix) {
  // This suffix and all after that are used for initialization of the various partitions. The unused suffixes lower than this value
  // are reserved for future operations on the MDT.
  private static final int PARTITION_INITIALIZATION_TIME_SUFFIX = 10; // corresponds to "010";
+  // we have max of 4 partitions (FILES, COL_STATS, BLOOM, RLI)
+  private static final List<String> VALID_PARTITION_INITIALIZATION_TIME_SUFFIXES = Arrays.asList("010","011","012","013");


Should this be dynamically generated based on the number of indexes / partition in the metadata table? For example, a new index can be added and update to this list may be missed.

for 0.14.0, since our MDT partitions are static, I did not want to fix that yet.

Makes sense. We should still track this as we may add new MDT partition in the future.

yihua · 2023-09-02T00:33:37Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

-    validInstantTimestamps.add(createIndexInitTimestamp(SOLO_COMMIT_TIMESTAMP, PARTITION_INITIALIZATION_TIME_SUFFIX));
+    metadataMetaClient.getActiveTimeline().getDeltaCommitTimeline().filterCompletedInstants()
+        .filter(instant ->  instant.getTimestamp().startsWith(SOLO_COMMIT_TIMESTAMP))
+        .getInstants().forEach(instant -> validInstantTimestamps.add(instant.getTimestamp()));


Could the suffix (from VALID_PARTITION_INITIALIZATION_TIME_SUFFIXES) be added to the completed commit, not SOLO_COMMIT_TIMESTAMP, as valid MDT initialization instant times?

E.g., when MDT is initialized for an existing data table with completed commits.

yeah, I also bumped into the same. its already taken care here

hudi/hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

Line 1301 in 2196694

public static boolean isValidInstant(HoodieInstant instant) {

hudi-bot · 2023-09-06T02:24:54Z

CI report:

0f68f4c Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

yihua · 2023-09-06T17:15:11Z

hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java

@@ -153,6 +153,8 @@ static boolean isValidSuffix(String suffix) {
  // This suffix and all after that are used for initialization of the various partitions. The unused suffixes lower than this value
  // are reserved for future operations on the MDT.
  private static final int PARTITION_INITIALIZATION_TIME_SUFFIX = 10; // corresponds to "010";
+  // we have max of 4 partitions (FILES, COL_STATS, BLOOM, RLI)
+  private static final List<String> VALID_PARTITION_INITIALIZATION_TIME_SUFFIXES = Arrays.asList("010","011","012","013");


Makes sense. We should still track this as we may add new MDT partition in the future.

1. This commit allows users to disable metadata using write configs cleanly. 2. Valid instants consideration while reading from MDT is solid now. We are going to treat any special instant time (that has additional suffix compared to DT's commit time) as valid. Especially with MDT partition initialization, the suffix is dynamic, and so we can't really find exact match. So, might have to go with total instant time length and treat all special instant times as valid ones. In the LogRecordReader, we will first ignore any uncommitted instants. And then if it's completed in MDT timeline, we check w/ the instantRange. So it should be fine to return true for any special instant times.

nsivabalan added priority:blocker release-0.14.0 labels Aug 26, 2023

nsivabalan changed the title ~~[HUDI-6759] Fixing valid instants in MDT~~ [HUDI-6397] [HUDI-6759] Fixing misc bugs w/ metadata table Aug 26, 2023

nsivabalan force-pushed the validInstantsFixForMDT branch from dc04cfe to f391322 Compare August 26, 2023 22:08

danny0405 reviewed Aug 28, 2023

View reviewed changes

hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/table/HoodieSparkTable.java Show resolved Hide resolved

danny0405 reviewed Aug 28, 2023

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/table/log/InstantRange.java Outdated Show resolved Hide resolved

nsivabalan force-pushed the validInstantsFixForMDT branch from f391322 to 98d5cfb Compare August 30, 2023 23:04

yihua reviewed Sep 2, 2023

View reviewed changes

nsivabalan added 6 commits September 5, 2023 15:41

Fixing valid instants in MDT

0bf0248

Fixing metadata deletion after enable

8b3848a

Addressing feedback

91aaa3c

fixing compilation error

fc6499e

Fixing test failurs

55f6f51

Fixing test failures

0f68f4c

nsivabalan force-pushed the validInstantsFixForMDT branch from 5bf20fa to 0f68f4c Compare September 5, 2023 22:41

yihua changed the title ~~[HUDI-6397] [HUDI-6759] Fixing misc bugs w/ metadata table~~ [HUDI-6397][HUDI-6759] Fixing misc bugs w/ metadata table Sep 6, 2023

yihua approved these changes Sep 6, 2023

View reviewed changes

yihua merged commit 48f89ae into apache:master Sep 6, 2023
26 of 28 checks passed

danny0405 mentioned this pull request Mar 5, 2024

[HUDI-7470] Compaction completed not need write to mdt if mdt is disable #10801

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-6397][HUDI-6759] Fixing misc bugs w/ metadata table #9546

[HUDI-6397][HUDI-6759] Fixing misc bugs w/ metadata table #9546

nsivabalan commented Aug 26, 2023 •

edited

Loading

yihua Sep 2, 2023

nsivabalan Sep 6, 2023

yihua Sep 2, 2023

nsivabalan Sep 2, 2023

yihua Sep 6, 2023

yihua Sep 2, 2023

yihua Sep 2, 2023

nsivabalan Sep 2, 2023

hudi-bot commented Sep 6, 2023

yihua Sep 6, 2023

[HUDI-6397][HUDI-6759] Fixing misc bugs w/ metadata table #9546

[HUDI-6397][HUDI-6759] Fixing misc bugs w/ metadata table #9546

Conversation

nsivabalan commented Aug 26, 2023 • edited Loading

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudi-bot commented Sep 6, 2023

CI report:

Choose a reason for hiding this comment

nsivabalan commented Aug 26, 2023 •

edited

Loading