[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

add unit test

hudi-bot · 2025-02-13T04:20:10Z

CI report:

b6d7d41 UNKNOWN
7801d77 UNKNOWN
dea1871 UNKNOWN
420e9db UNKNOWN
9c710e9 UNKNOWN
bee0cdf UNKNOWN
7b08e0b Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

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

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?

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?

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

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 from 9699515 to 082fd85 Compare February 13, 2025 01:06

Davis-Zhang-Onehouse added 5 commits February 12, 2025 18:28

fix SerDe implementation

26c97d8

add unit test

util changes done

db886e0

introduce getInstantContentStream API + refactor isEmpty

ceb9ddf

deprecate byte [] in deserilzation

fe35d55

deprecate byte [] in deserilzation

7b08e0b

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

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

View reviewed changes

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

hudi-bot commented Feb 13, 2025

the-other-tim-brown Feb 13, 2025

the-other-tim-brown Feb 13, 2025

the-other-tim-brown Feb 13, 2025

the-other-tim-brown Feb 13, 2025

the-other-tim-brown Feb 13, 2025

the-other-tim-brown Feb 13, 2025

the-other-tim-brown Feb 13, 2025

the-other-tim-brown Feb 13, 2025

the-other-tim-brown Feb 13, 2025

the-other-tim-brown Feb 13, 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

hudi-bot commented Feb 13, 2025

CI report:

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

Davis-Zhang-Onehouse commented Feb 11, 2025 •

edited

Loading