HDDS-13849. Refactor getTablePrefix function in SnapshotDiff flow by swamirishi · Pull Request #9235 · apache/ozone

swamirishi · 2025-11-01T21:21:07Z

What changes were proposed in this pull request?

Currently get table Prefix function is placed inside snapshotUtils. However the implementation should dictate the various table prefix. For every diff computation corresponding to a table DAG traversal is done for all tables redundantly making the snapshot diff operation do redundant sst file seeks and reads which could have been avoided.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13849

How was this patch tested?

Fixed existing unit tests

Change-Id: If068daeb7ca9bf3234614a29da90edcb9fb5778d

Change-Id: If064b64d1dfd624324090a946a33eab55535f28f

Change-Id: I3ed6575a96a12c349bb229fb3772c256a986f242

Change-Id: Iaddc93f6dc50a34dd5ac3eaac92b7400f4856ccf

Change-Id: I1eb91064f806d5cd5393c74597c727e41eb11742

swamirishi · 2025-11-10T16:27:46Z

@SaketaChalamchala can you review this

Change-Id: I98a74e0d6815f28792096f0992e1aca933706963

Copilot

Pull Request Overview

This PR refactors the snapshot diff flow by moving table prefix computation from a utility class to the metadata manager implementation level. The key improvement is eliminating redundant SST file filtering operations by centralizing table prefix management and changing the representation from Map<String, String> to a dedicated TablePrefixInfo class.

Key Changes:

Introduced TablePrefixInfo class to encapsulate table-to-prefix mappings
Moved table prefix computation from SnapshotUtils.getColumnFamilyToKeyPrefixMap() to OMMetadataManager.getTableBucketPrefix()
Updated SST filtering methods to accept Set<String> instead of List<String> for table lookups
Consolidated duplicate table cleanup methods in OmSnapshotManager

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`TablePrefixInfo.java`	New class encapsulating table prefix mappings with immutable map
`OMMetadataManager.java`	Added interface methods for computing table bucket prefixes
`OmMetadataManagerImpl.java`	Implemented table prefix computation with switch-based lookup
`SnapshotDiffManager.java`	Updated to use `TablePrefixInfo` and `Set<String>` for table lookups
`RocksDiffUtils.java`	Refactored SST filtering to use `TablePrefixInfo` and table sets
`RocksDBCheckpointDiffer.java`	Updated DAG traversal to filter by table set
`KeyManagerImpl.java`	Modified to use table-based prefix lookup
`OmSnapshotManager.java`	Consolidated three separate table cleanup methods into one
Test files	Updated mocks and assertions to use new types

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-10T20:49:14Z

...e/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java

      LOG.info("{} actual row count={}, expectedCount={}", table.getName(),
          count.get(), expectedCount);
    });
+    System.out.println("Swaminathan \t" + count.get() + "\t" + expectedCount);


Debug print statement should be removed before merging to production. Use the existing LOG.info statement instead or remove entirely.

Suggested change

System.out.println("Swaminathan \t" + count.get() + "\t" + expectedCount);

// Debug print statement removed; information is already logged above.

please fix this

Copilot · 2025-11-10T20:49:14Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/StringUtils.java

+  public static String getFirstNChars(String str, int n) {
+    if (str == null || str.length() < n) {
+      return str;
+    }
+    return str.substring(0, n);
+  }


The method returns the original string when str.length() < n, but should return the full string only when it's shorter. When str is null, returning null may cause NullPointerExceptions in callers. Consider returning an empty string for null input or documenting this behavior clearly.

this is fine.

Copilot · 2025-11-10T20:49:14Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java

    return false;
  }

+  // NOTE: Update both getTableBucketPrefixInfo(volume, bucket) & getTableBucketPrefix(tableName, volume, bucket)


The comment references 'getTableBucketPrefixInfo' but the actual method name is 'getTableBucketPrefix' (without 'Info'). Update the comment to match the actual method signature.

Suggested change

// NOTE: Update both getTableBucketPrefixInfo(volume, bucket) & getTableBucketPrefix(tableName, volume, bucket)

// NOTE: Update both getTableBucketPrefix(volume, bucket) & getTableBucketPrefix(tableName, volume, bucket)

Copilot · 2025-11-10T20:49:15Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java

+    case DELETED_DIR_TABLE:
+    case OPEN_FILE_TABLE:
+      return getBucketKeyPrefixFSO(volume, bucket);
+    default:


Returning an empty string as default for unrecognized table names could silently mask configuration errors. Consider logging a warning when an unknown table name is encountered to aid debugging.

Suggested change

default:

default:

LOG.warn("Unknown table name '{}' passed to getTableBucketPrefix (volume='{}', bucket='{}'). Returning empty string.", tableName, volume, bucket);

jojochuang

Apart from cosmetic issues and missing javadocs, this looks good.

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java

jojochuang · 2025-11-10T21:39:16Z

...e/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java

      LOG.info("{} actual row count={}, expectedCount={}", table.getName(),
          count.get(), expectedCount);
    });
+    System.out.println("Swaminathan \t" + count.get() + "\t" + expectedCount);


please fix this

jojochuang · 2025-11-10T21:39:36Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/StringUtils.java

+  public static String getFirstNChars(String str, int n) {
+    if (str == null || str.length() < n) {
+      return str;
+    }
+    return str.substring(0, n);
+  }


this is fine.

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java

Change-Id: I06301f780ff7fff2ab652722872b1707501d61ec

jojochuang

LGTM

Change-Id: I13eb719258714697f875d0b64d38e1185ba75db1

swamirishi · 2025-11-11T00:09:05Z

Thank you @jojochuang for reviewing the patch

adoroszlai · 2025-11-11T06:45:41Z

@swamirishi please set fix version when resolving Jira issue after PR merge

HDDS-13849. Refactor getTablePrefix function in SnapshotDiff flow

4fd3b0e

Change-Id: If068daeb7ca9bf3234614a29da90edcb9fb5778d

swamirishi requested review from jojochuang, sadanand48 and smengcl November 3, 2025 04:12

swamirishi added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Nov 3, 2025

swamirishi mentioned this pull request Nov 3, 2025

HDDS-13452. Prevent snapshot defrag from happening before upgrade finalization #9240

Merged

swamirishi added 4 commits November 3, 2025 00:27

Merge remote-tracking branch 'apache/master' into HEAD

c46e1ae

Change-Id: If064b64d1dfd624324090a946a33eab55535f28f

HDDS-13849. Fix test

298ae36

Change-Id: I3ed6575a96a12c349bb229fb3772c256a986f242

Merge remote-tracking branch 'apache/master' into HEAD

974b5c2

Change-Id: Iaddc93f6dc50a34dd5ac3eaac92b7400f4856ccf

Merge remote-tracking branch 'apache/master' into HEAD

91c143a

Change-Id: I1eb91064f806d5cd5393c74597c727e41eb11742

swamirishi marked this pull request as ready for review November 6, 2025 17:53

swamirishi mentioned this pull request Nov 10, 2025

HDDS-13867. SnapshotDiff delta file computation should happen based on LocalDataYaml file #9268

Merged

Merge remote-tracking branch 'apache/master' into HEAD

894a047

Change-Id: I98a74e0d6815f28792096f0992e1aca933706963

jojochuang requested a review from Copilot November 10, 2025 20:40

Copilot AI reviewed Nov 10, 2025

View reviewed changes

jojochuang reviewed Nov 10, 2025

View reviewed changes

HDDS-13849. Address review comments

6fbf8c7

Change-Id: I06301f780ff7fff2ab652722872b1707501d61ec

jojochuang approved these changes Nov 10, 2025

View reviewed changes

HDDS-13849. Address review comments

1507659

Change-Id: I13eb719258714697f875d0b64d38e1185ba75db1

swamirishi force-pushed the HDDS-13849 branch from ea64602 to 1507659 Compare November 10, 2025 22:56

swamirishi merged commit ff41ce6 into apache:master Nov 11, 2025
43 checks passed

	System.out.println("Swaminathan \t" + count.get() + "\t" + expectedCount);
	// Debug print statement removed; information is already logged above.

	// NOTE: Update both getTableBucketPrefixInfo(volume, bucket) & getTableBucketPrefix(tableName, volume, bucket)
	// NOTE: Update both getTableBucketPrefix(volume, bucket) & getTableBucketPrefix(tableName, volume, bucket)

	default:
	default:
	LOG.warn("Unknown table name '{}' passed to getTableBucketPrefix (volume='{}', bucket='{}'). Returning empty string.", tableName, volume, bucket);

Conversation

swamirishi commented Nov 1, 2025

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

swamirishi commented Nov 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

jojochuang Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

jojochuang Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

swamirishi Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

swamirishi Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jojochuang Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

jojochuang Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

swamirishi commented Nov 11, 2025

Uh oh!

adoroszlai commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants