[HUDI-8992] Deprecate all byte array usage in metadata deserialization path #12826

Davis-Zhang-Onehouse · 2025-02-11T03:32:20Z

Reviewers please start reviewing from commit with title "util changes done", the upstream PR is tracked in #12829

Change Logs

For all metadata deserialization now we change them to use input streams. A few minor place that requires more code refactoring we need to follow up separately.

Also fix the logic of checking if a given instant file is empty, previously we load file to byte array and check length, now we check file stats which is much light weight.

Impact

Allows for reading of large commit metadata without OOM.

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

low

Documentation Update

NA

Contributor's checklist

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

the-other-tim-brown · 2025-02-13T22:34:29Z

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/MetadataConversionUtils.java

@@ -394,4 +423,12 @@ public static <T extends SpecificRecordBase> byte[] convertCommitMetadataToJsonB
      throw new HoodieIOException("Failed to convert to JSON.", e);
    }
  }
+
+  public static boolean isEmptyStream(InputStream inputStream) throws IOException {


It looks like this method is unused?

the-other-tim-brown · 2025-02-13T22:34:50Z

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/MetadataConversionUtils.java

+    }
+    return JsonUtils.getObjectMapper().convertValue(replaceCommitMetadata, HoodieReplaceCommitMetadata.class);
+  }
+


The method below this one is now showing as unused, can it be cleaned up?

the-other-tim-brown · 2025-02-13T22:45:32Z

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/TimelineMetadataUtils.java

  }

-  public static HoodieCleanerPlan deserializeCleanerPlan(byte[] bytes) throws IOException {
-    return deserializeAvroMetadata(bytes, HoodieCleanerPlan.class);
+  public static HoodieCleanMetadata deserializeHoodieCleanMetadataLegacy(byte[] bytes) throws IOException {


There are only 2 callers of this and it looks like they can both be migrated to use the content stream.

the-other-tim-brown · 2025-02-13T22:49:58Z

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/MetadataConversionUtils.java

    switch (actionType) {
      case HoodieTimeline.CLEAN_ACTION: {
        archivedMetaWrapper.setHoodieCleanMetadata(CleanerUtils.getCleanerMetadata(metaClient, instantDetails.get()));
-        archivedMetaWrapper.setHoodieCleanerPlan(CleanerUtils.getCleanerPlan(metaClient, planBytes.get()));
+        archivedMetaWrapper.setHoodieCleanerPlan(CleanerUtils.getCleanerPlanLegacy(metaClient, planBytes.get()));


We could replace all the "legacy" methods by wrapping the bytes in an input stream. This seems to be the main non-test class where these methods are used so we can also try to remove those methods after cleaning this up. This could help avoid confusion for future developers.

the-other-tim-brown · 2025-02-13T22:51:42Z

hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestCleansCommand.java

@@ -202,7 +202,7 @@ private Long getLatestCleanTimeTakenInMillis() throws IOException {
    HoodieInstant clean = timeline.getReverseOrderedInstants().findFirst().orElse(null);
    if (clean != null) {
      HoodieCleanMetadata cleanMetadata =
-          TimelineMetadataUtils.deserializeHoodieCleanMetadata(timeline.getInstantDetails(clean).get());
+          TimelineMetadataUtils.deserializeHoodieCleanMetadataLegacy(timeline.getInstantDetails(clean).get());


can this

Suggested change

TimelineMetadataUtils.deserializeHoodieCleanMetadataLegacy(timeline.getInstantDetails(clean).get());

TimelineMetadataUtils.deserializeHoodieCleanMetadata(timeline.getInstantContentStream(clean));

the-other-tim-brown · 2025-02-13T23:16:46Z

...on/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/ArchivedTimelineV2.java

+
+  @Override
+  public boolean isEmpty(HoodieInstant instant) {
+    return TimelineUtils.isEmpty(metaClient, instant);


Similarly here, do we want to reference the readCommits?

applied the same fix - keep the behavior the same as before.

the-other-tim-brown · 2025-02-13T23:19:28Z

hudi-common/src/main/java/org/apache/hudi/common/util/CleanerUtils.java

+  public static HoodieCleanMetadata getCleanerMetadata(HoodieTableMetaClient metaClient, Option<InputStream> details)
+      throws IOException {
+    CleanMetadataMigrator metadataMigrator = new CleanMetadataMigrator(metaClient);
+    HoodieCleanMetadata cleanMetadata = TimelineMetadataUtils.deserializeHoodieCleanMetadata(details);
+    return metadataMigrator.upgradeToLatest(cleanMetadata, cleanMetadata.getVersion());
+  }
+
+  public static HoodieCleanMetadata getCleanerMetadataFromInputStream(HoodieTableMetaClient metaClient, Option<InputStream> in)
+      throws IOException {
+    CleanMetadataMigrator metadataMigrator = new CleanMetadataMigrator(metaClient);
+    HoodieCleanMetadata cleanMetadata = TimelineMetadataUtils.deserializeHoodieCleanMetadata(in);
+    return metadataMigrator.upgradeToLatest(cleanMetadata, cleanMetadata.getVersion());
+  }


These two methods look like they are the same?

oh yes, fixed.

the-other-tim-brown · 2025-02-13T23:20:41Z

hudi-common/src/main/java/org/apache/hudi/common/util/ClusteringUtils.java

@@ -190,22 +190,21 @@ private static Option<HoodieRequestedReplaceMetadata> getRequestedReplaceMetadat
      String action = factory instanceof InstantGeneratorV2 ? HoodieTimeline.CLUSTERING_ACTION : HoodieTimeline.REPLACE_COMMIT_ACTION;
      requestedInstant = factory.createNewInstant(HoodieInstant.State.REQUESTED, action, pendingReplaceOrClusterInstant.requestedTime());
    }
-    Option<byte[]> content = Option.empty();
+    if (timeline.isEmpty(requestedInstant)) {


Can we move this to the error handling?

the-other-tim-brown · 2025-02-13T23:28:37Z

hudi-common/src/main/java/org/apache/hudi/common/util/CompactionUtils.java

+    }
+  }
+
+  public static HoodieCompactionPlan getCompactionPlanFromInputStream(HoodieTableMetaClient metaClient, Option<InputStream> in) {


This seems similar to the method at line 196

the-other-tim-brown · 2025-02-13T23:31:16Z

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

@@ -1094,7 +1094,7 @@ private static void reAddLogFilesFromRollbackPlan(HoodieTableMetaClient dataTabl
    HoodieInstant requested = factory.getRollbackRequestedInstant(rollbackInstant);
    try {
      HoodieRollbackPlan rollbackPlan = TimelineMetadataUtils.deserializeAvroMetadata(
-          dataTableMetaClient.getActiveTimeline().readRollbackInfoAsBytes(requested).get(), HoodieRollbackPlan.class);


readRollbackInfoAsBytes looks like it is no longer used in the codebase and can be removed

add unit test

Davis-Zhang-Onehouse · 2025-02-21T22:13:17Z

hudi-hadoop-common/src/test/java/org/apache/hudi/common/table/TestTableSchemaResolver.java

@@ -242,145 +220,6 @@ public String toString() {
    }
  }

-  static Stream<SchemaEvolutionTestCase> schemaEvolutionTestCases() {


remove the test as it failed due to mock does not match the updated code change. Since we remove all those tests in #12781, I removed them here as well to unblock java CI.

hudi-bot · 2025-02-23T22:38:22Z

CI report:

b6d7d41 UNKNOWN
7801d77 UNKNOWN
dea1871 UNKNOWN
420e9db UNKNOWN
9c710e9 UNKNOWN
bee0cdf UNKNOWN
b68d5ed UNKNOWN
1f76064 UNKNOWN
ee5d3e7 UNKNOWN
4f05c77 UNKNOWN
120cd75 UNKNOWN
9555bd5 UNKNOWN
25fa7e9 UNKNOWN
661b6e4 UNKNOWN
cf4c277 UNKNOWN
005fd0a Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

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

Davis-Zhang-Onehouse force-pushed the HUDI-8992-migrate2 branch from a1c91f0 to 8552c5d Compare February 11, 2025 03:33

github-actions bot added the size:L PR with lines of changes in (300, 1000] label Feb 11, 2025

Davis-Zhang-Onehouse force-pushed the HUDI-8992-migrate2 branch from 8552c5d to 4e48fd8 Compare February 12, 2025 02:26

github-actions bot added size:XL PR with lines of changes > 1000 and removed size:L PR with lines of changes in (300, 1000] labels Feb 12, 2025

Davis-Zhang-Onehouse force-pushed the HUDI-8992-migrate2 branch 12 times, most recently from bee0cdf to 9699515 Compare February 13, 2025 00:38

Davis-Zhang-Onehouse changed the title ~~Hudi 8992 migrate2~~ [HUDI-8992] Deprecate all byte array usage in metadata deserialization path Feb 13, 2025

Davis-Zhang-Onehouse force-pushed the HUDI-8992-migrate2 branch 2 times, most recently from 082fd85 to 7b08e0b Compare February 13, 2025 02:47

the-other-tim-brown reviewed Feb 13, 2025

View reviewed changes

Davis-Zhang-Onehouse force-pushed the HUDI-8992-migrate2 branch 9 times, most recently from 12cd738 to 2880aca Compare February 20, 2025 23:18

Davis-Zhang-Onehouse force-pushed the HUDI-8992-migrate2 branch 5 times, most recently from 680f56a to 17caf9f Compare February 21, 2025 17:07

Davis-Zhang-Onehouse added 10 commits February 21, 2025 10:13

fix SerDe implementation

31ea948

add unit test

add unit test for schema validation

4d6938b

remove unnecessary avroMap.remove(null) code

6ec4242

util changes done

8ea36b5

introduce getInstantContentStream API + refactor isEmpty

ffba1b7

deprecate byte [] in deserilzation

fa8cd22

deprecate byte [] in deserilzation

a842780

address PR comments Pt1

f0c80c9

address PR comments Pt2 - lazily evaluate file emptiness

b59e519

fix getContentStream to avoid reading file stats

165b435

Davis-Zhang-Onehouse force-pushed the HUDI-8992-migrate2 branch 3 times, most recently from 670b377 to 81b6e74 Compare February 21, 2025 22:11

Davis-Zhang-Onehouse commented Feb 21, 2025

View reviewed changes

Davis-Zhang-Onehouse force-pushed the HUDI-8992-migrate2 branch from 360abad to cf4c277 Compare February 22, 2025 03:18

fix compilation issue

74e6118

Davis-Zhang-Onehouse force-pushed the HUDI-8992-migrate2 branch from cf4c277 to 74e6118 Compare February 22, 2025 03:22

fix cleaner version + minor code refactor

005fd0a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-8992] Deprecate all byte array usage in metadata deserialization path #12826

[HUDI-8992] Deprecate all byte array usage in metadata deserialization path #12826

Davis-Zhang-Onehouse commented Feb 11, 2025 •

edited

Loading

the-other-tim-brown Feb 13, 2025

Davis-Zhang-Onehouse Feb 20, 2025

the-other-tim-brown Feb 13, 2025

Davis-Zhang-Onehouse Feb 20, 2025

the-other-tim-brown Feb 13, 2025

Davis-Zhang-Onehouse Feb 20, 2025

the-other-tim-brown Feb 13, 2025

Davis-Zhang-Onehouse Feb 20, 2025

the-other-tim-brown Feb 13, 2025

Davis-Zhang-Onehouse Feb 20, 2025

the-other-tim-brown Feb 13, 2025

Davis-Zhang-Onehouse Feb 20, 2025

the-other-tim-brown Feb 13, 2025

Davis-Zhang-Onehouse Feb 20, 2025

the-other-tim-brown Feb 13, 2025

Davis-Zhang-Onehouse Feb 20, 2025

the-other-tim-brown Feb 13, 2025

Davis-Zhang-Onehouse Feb 20, 2025

the-other-tim-brown Feb 13, 2025

Davis-Zhang-Onehouse Feb 20, 2025

Davis-Zhang-Onehouse Feb 21, 2025

hudi-bot commented Feb 23, 2025

	TimelineMetadataUtils.deserializeHoodieCleanMetadataLegacy(timeline.getInstantDetails(clean).get());
	TimelineMetadataUtils.deserializeHoodieCleanMetadata(timeline.getInstantContentStream(clean));

[HUDI-8992] Deprecate all byte array usage in metadata deserialization path #12826

Are you sure you want to change the base?

[HUDI-8992] Deprecate all byte array usage in metadata deserialization path #12826

Conversation

Davis-Zhang-Onehouse commented Feb 11, 2025 • edited Loading

Reviewers please start reviewing from commit with title "util changes done", the upstream PR is tracked in #12829

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

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

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 Feb 23, 2025

CI report:

Davis-Zhang-Onehouse commented Feb 11, 2025 •

edited

Loading