HDDS-12835. Recon - Improve NSSummary tasks execution using custom lightweight codec. by devmadhuu · Pull Request #8541 · apache/ozone

devmadhuu · 2025-06-02T10:55:10Z

What changes were proposed in this pull request?

This PR is to add custom light weight codecs for Recon OM DB to improve the performance of Recon OM tasks including NSSummary tasks execution using custom lightweight codec, as most underlying Recon OM tasks like OMDBInsightTask , NSSummary tasks etc are where it just need very few fields like name, object id, size and parent object Id from KeyInfoTable. Lightweight custom codecs are excluding the de-serialization of ACLs and skipping them altogether as Recon OM tasks doesn't use them.

What is the link to the Apache JIRA

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

How was this patch tested?

This PR is being tested with existing junit and integration tests. Few additional test cases development is in progress.

…vement.

adoroszlai · 2025-06-03T08:55:53Z

Thanks @devmadhuu for fixing the javadoc. Note there are several test failures due to:

NullPointerException: Cannot invoke "org.apache.ratis.util.function.CheckedFunction.apply(Object)" because "this.backward" is null
	at org.apache.hadoop.hdds.utils.db.DelegatedCodec.toCodecBuffer(DelegatedCodec.java:78)
	at org.apache.hadoop.hdds.utils.db.Codec.toDirectCodecBuffer(Codec.java:66)
	at org.apache.hadoop.hdds.utils.db.TypedTable.put(TypedTable.java:138)
	at org.apache.hadoop.ozone.recon.api.TestOmDBInsightEndPoint.setUpOmData(TestOmDBInsightEndPoint.java:417)
	at org.apache.hadoop.ozone.recon.api.TestOmDBInsightEndPoint.setUp(TestOmDBInsightEndPoint.java:305)

devmadhuu · 2025-06-03T10:08:40Z

Thanks @devmadhuu for fixing the javadoc. Note there are several test failures due to:

NullPointerException: Cannot invoke "org.apache.ratis.util.function.CheckedFunction.apply(Object)" because "this.backward" is null
	at org.apache.hadoop.hdds.utils.db.DelegatedCodec.toCodecBuffer(DelegatedCodec.java:78)
	at org.apache.hadoop.hdds.utils.db.Codec.toDirectCodecBuffer(Codec.java:66)
	at org.apache.hadoop.hdds.utils.db.TypedTable.put(TypedTable.java:138)
	at org.apache.hadoop.ozone.recon.api.TestOmDBInsightEndPoint.setUpOmData(TestOmDBInsightEndPoint.java:417)
	at org.apache.hadoop.ozone.recon.api.TestOmDBInsightEndPoint.setUp(TestOmDBInsightEndPoint.java:305)

Yeah @adoroszlai , I am aware of those and looking into it already.

adoroszlai · 2025-06-04T07:12:19Z

+import org.apache.hadoop.ozone.protocolPB.OMPBHelper;
+
+/**
+ * OM database definitions.


Please don't duplicate javadoc from OMDBDefinition. Instead, add a short explanation of how this is different and why it is needed.

adoroszlai · 2025-06-04T07:15:35Z

+  /** volumeTable: /volume :- VolumeInfo. */
+  public static final DBColumnFamilyDefinition<String, OmVolumeArgs> VOLUME_TABLE_DEF
+      = new DBColumnFamilyDefinition<>(VOLUME_TABLE,
+          StringCodec.get(),
+          OmVolumeArgs.getCodec());


Can remove and use from OMDBDefinition?

adoroszlai · 2025-06-04T08:19:08Z

+   * Parses BucketInfo protobuf and creates OmBucketInfo without deserializing ACL list.
+   * @param bucketInfo
+   * @return instance of OmBucketInfo
+   */
+  public static OmBucketInfo getOmBucketInfoFromProtobuf(OzoneManagerProtocolProtos.BucketInfo bucketInfo) {
+    OmBucketInfo.Builder obib = OmBucketInfo.newBuilder()
+        .setVolumeName(bucketInfo.getVolumeName())
+        .setBucketName(bucketInfo.getBucketName())
+        .setIsVersionEnabled(bucketInfo.getIsVersionEnabled())
+        .setStorageType(StorageType.valueOf(bucketInfo.getStorageType()))
+        .setCreationTime(bucketInfo.getCreationTime())
+        .setUsedBytes(bucketInfo.getUsedBytes())
+        .setModificationTime(bucketInfo.getModificationTime())
+        .setQuotaInBytes(bucketInfo.getQuotaInBytes())
+        .setUsedNamespace(bucketInfo.getUsedNamespace())
+        .setQuotaInNamespace(bucketInfo.getQuotaInNamespace());


Instead of re-implementing most of the conversion logic without ACL, please:

move this to OmBucketInfo as a helper that returns OmBucketInfo.Builder

let both "full" and "reduced" conversion call the helper and then build(), with "full" adding further logic between the two steps

Apply the same to OmKeyInfo, etc.

@adoroszlai These definition is specific for use case, to keep minimal whatever is required. There can be multiple variation of building the object. eg:

may be only need decode parentId and usedSpace

may be another need blockList information to be decoded

So I think keeping all in OMBucketInfo is not correct as it can keep changing, and these are to be used within that context, else can have issue.
Code duplicate handing / giving partial method is confusing as it does not define the usecase of the method. So we need keep more close to where its being used, like Recon.

Which makes it trivial to forget about Recon's implementation when changing OMBucketInfo...

Instead of moving to Recon, define the use case in OMBucketInfo instead of just a generic name "...Partial".

Pls review again. I have tried to refactor by reusing as much as possible.

patch updated

adoroszlai · 2025-06-10T15:56:36Z

Thanks @devmadhuu for updating the patch.

sumitagrawl · 2025-06-16T06:35:49Z

+      = new DBColumnFamilyDefinition<>(DELETED_DIR_TABLE, StringCodec.get(), CUSTOM_CODEC_FOR_KEY_TABLE);
+
+  //---------------------------------------------------------------------------
+  public static final Map<String, DBColumnFamilyDefinition<?, ?>> COLUMN_FAMILIES


We should avoid creating another map of all, as if any new changes happens, it needs to be updated to Recon Map. The chances of missing is high. Instead, any get operation should get first from this map and then OM's map.

szetszwo

@devmadhuu , thanks for working on this! Please see the comments inlined.

szetszwo · 2025-06-17T21:44:39Z

-  public static OmBucketInfo getFromProtobuf(BucketInfo bucketInfo,
-      BucketLayout buckLayout) {
-    Builder obib = OmBucketInfo.newBuilder()
+  public static Builder newBuilderFromProtobufPartial(BucketInfo bucketInfo) {


Handle all fields in one method follow the same order as in the proto.

Add a boolean to include/exclude the large fields.

Add javadoc.

/** * Create a builder from the given proto. * @param includeLargeFields Omitted the large fields: acl, metadata, beinfo. */ public static Builder newBuilder(BucketInfo bucketInfo, boolean includeLargeFields) { final Builder builder = OmBucketInfo.newBuilder() .setVolumeName(bucketInfo.getVolumeName()) .setBucketName(bucketInfo.getBucketName()); if (includeLargeFields) { // acl builder.setAcls(bucketInfo.getAclsList().stream().map( OzoneAcl::fromProtobuf).collect(Collectors.toList())); } builder.setIsVersionEnabled(bucketInfo.getIsVersionEnabled()) .setStorageType(StorageType.valueOf(bucketInfo.getStorageType())) .setCreationTime(bucketInfo.getCreationTime()); if (includeLargeFields) { // metadata builder.addAllMetadata(KeyValueUtil.getFromProtobuf(bucketInfo.getMetadataList())); if (bucketInfo.hasBeinfo()) { // beinfo builder.setBucketEncryptionKey(OMPBHelper.convert(bucketInfo.getBeinfo())); } } if (bucketInfo.hasObjectID()) { builder.setObjectID(bucketInfo.getObjectID()); } if (bucketInfo.hasUpdateID()) { builder.setUpdateID(bucketInfo.getUpdateID()); } builder.setModificationTime(bucketInfo.getModificationTime()); if (bucketInfo.hasSourceVolume()) { builder.setSourceVolume(bucketInfo.getSourceVolume()); } if (bucketInfo.hasSourceBucket()) { builder.setSourceBucket(bucketInfo.getSourceBucket()); } builder.setUsedBytes(bucketInfo.getUsedBytes()) .setQuotaInBytes(bucketInfo.getQuotaInBytes()) .setQuotaInNamespace(bucketInfo.getQuotaInNamespace()) .setUsedNamespace(bucketInfo.getUsedNamespace()) if (bucketInfo.hasBucketLayout()) { builder.setBucketLayout(BucketLayout.fromProto(bucketInfo.getBucketLayout())); } if (bucketInfo.hasOwner()) { builder.setOwner(bucketInfo.getOwner()); } if (bucketInfo.hasDefaultReplicationConfig()) { builder.setDefaultReplicationConfig( DefaultReplicationConfig.fromProto(bucketInfo.getDefaultReplicationConfig())); } return builder; }

szetszwo · 2025-06-17T22:04:39Z

-   * @return instance of OmDirectoryInfo
-   */
-  public static OmDirectoryInfo getFromProtobuf(DirectoryInfo dirInfo) {
+  public static Builder newBuilderFromProtobufPartial(DirectoryInfo dirInfo) {


Change it similarly as OmBucketInfo, i.e. use one method to include/exclude large fields.

szetszwo · 2025-06-17T22:05:22Z

+        .setName(dirInfo.getName())
+        .setCreationTime(dirInfo.getCreationTime())
+        .setModificationTime(dirInfo.getModificationTime());
    if (dirInfo.getMetadataList() != null) {


Do we want to include metadata in this case?

szetszwo · 2025-06-17T22:05:36Z

      opib.addAllMetadata(KeyValueUtil
-              .getFromProtobuf(dirInfo.getMetadataList()));
+          .getFromProtobuf(dirInfo.getMetadataList()));


Combine two lines.

szetszwo · 2025-06-17T22:06:27Z

-    for (KeyLocationList keyLocationList : keyInfo.getKeyLocationListList()) {
+    for (OzoneManagerProtocolProtos.KeyLocationList keyLocationList : keyInfo.getKeyLocationListList()) {


Revert this change.

szetszwo · 2025-06-17T22:18:06Z

+  /**
+   * Parses BucketInfo protobuf and creates OmBucketInfo without deserializing ACL list.
+   * @param bucketInfo
+   * @return instance of OmBucketInfo
+   */


This javadoce does not provide any useful information and generate "tag description is missing" warning. Please remove it or rewrite it.

szetszwo · 2025-06-17T22:18:13Z

+  /**
+   * Parses DirectoryInfo protobuf and creates OmPrefixInfo.
+   * @param dirInfo
+   * @return instance of OmDirectoryInfo
+   */


This javadoce does not provide any useful information and generate "tag description is missing" warning. Please remove it or rewrite it.

szetszwo · 2025-06-17T22:22:10Z

+  private static final Codec<OmBucketInfo> CUSTOM_CODEC_FOR_BUCKET_TABLE = new DelegatedCodec<>(
+      Proto2Codec.get(OzoneManagerProtocolProtos.BucketInfo.getDefaultInstance()),
+      ReconOMDBDefinition::getOmBucketInfoFromProtobuf,
+      null,


Use OmBucketInfo::getProtobuf ?

Recon does not write or serialize data to bucket table. Only needed for forward.

szetszwo · 2025-06-17T22:24:34Z

+  public static final DBColumnFamilyDefinition<String, OmBucketInfo> BUCKET_TABLE_DEF
+      = new DBColumnFamilyDefinition<>(BUCKET_TABLE, StringCodec.get(), CUSTOM_CODEC_FOR_BUCKET_TABLE);
+
+  public static final Codec<OmKeyInfo> CUSTOM_CODEC_FOR_KEY_TABLE = new DelegatedCodec<>(


It is not only for KEY_TABLE. Let's rename it to CUSTOM_OM_KEY_INFO_CODEC.

szetszwo · 2025-06-17T22:31:50Z

+      Table<String, OmDirectoryInfo> dirTable = omMetadataManager.getStore()
+          .getTable(DIRECTORY_TABLE, StringCodec.get(), CUSTOM_CODEC_FOR_DIR_TABLE, TableCache.CacheType.NO_CACHE);


Use DEF:

final Table<String, OmDirectoryInfo> dirTable = ReconOMDBDefinition.DIRECTORY_TABLE_DEF .getTable(omMetadataManager.getStore());

devmadhuu · 2025-06-27T13:46:18Z

Test failures in this PR doesn't seem related to the PR change.

sumitagrawl

IMO, this PR is no longer needed as,

ACL issue is fixed with most probable size is "1" only
metadata size is mostly "0" except for eTag case where normally size is "1"
So this may not provide much signification improvement in performance.

devmadhuu · 2025-07-11T12:19:21Z

IMO, this PR is no longer needed as,

1. ACL issue is fixed with most probable size is "1" only

2. metadata size is mostly "0" except for eTag case where normally size is "1"
   So this may not provide much signification improvement in performance.

Ok, then we can close this PR with this understanding.

devmadhuu force-pushed the HDDS-12835 branch from 3179020 to 4df5023 Compare June 2, 2025 12:23

devmadhuu changed the title ~~HDDS-12835.Recon - Improve NSSummary tasks execution using custom lightweight codec.~~ HDDS-12835. Recon - Improve NSSummary tasks execution using custom lightweight codec. Jun 2, 2025

jojochuang requested a review from sumitagrawl June 2, 2025 16:18

Devesh Kumar Singh added 2 commits June 3, 2025 12:16

HDDS-12835. Added test cases to validate and assert performance impro…

643c64a

…vement.

Merge remote-tracking branch 'origin/master' into HDDS-12835

89c6543

devmadhuu force-pushed the HDDS-12835 branch from 4df5023 to 89c6543 Compare June 3, 2025 06:49

devmadhuu marked this pull request as ready for review June 3, 2025 06:49

HDDS-12835. PMD issues.

cc59702

adoroszlai reviewed Jun 3, 2025

View reviewed changes

Comment thread ...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/codec/OMDBDefinitionBase.java Outdated

adoroszlai added the recon label Jun 3, 2025

HDDS-12835. javadoc issues.

5344366

adoroszlai marked this pull request as draft June 3, 2025 10:13

Devesh Kumar Singh added 2 commits June 3, 2025 21:48

HDDS-12835. Integration test issues.

93f8615

HDDS-12835. Integration test issues.

3cbdc84

devmadhuu marked this pull request as ready for review June 4, 2025 01:26

devmadhuu requested a review from adoroszlai June 4, 2025 04:53

adoroszlai removed their request for review June 4, 2025 05:30

adoroszlai previously requested changes Jun 4, 2025

View reviewed changes

Devesh Kumar Singh added 2 commits June 10, 2025 13:48

HDDS-12835. Refactoring the code.

eb756b2

HDDS-12835. Updated javadoc of ReconOMDBDefinition class.

222db13

devmadhuu requested a review from adoroszlai June 10, 2025 12:04

Devesh Kumar Singh added 2 commits June 10, 2025 17:58

HDDS-12835. Updated javadoc of ReconOMDBDefinition class.

2cc735f

HDDS-12835. Updated javadoc of ReconOMDBDefinition class.

d53a2c5

adoroszlai requested review from ArafatKhan2198 and removed request for adoroszlai June 10, 2025 15:56

sumitagrawl reviewed Jun 16, 2025

View reviewed changes

Devesh Kumar Singh added 4 commits June 16, 2025 16:16

HDDS-12835. Refactored ReconOMDBDefinition class.

9772ec8

HDDS-12835. PMD fixes.

ffe6536

HDDS-12835. findbug fixes.

2cc3409

HDDS-12835. pmd and checkstyle fixes.

9750045

devmadhuu requested review from adoroszlai and sumitagrawl June 17, 2025 04:19

adoroszlai requested a review from szetszwo June 17, 2025 16:49

szetszwo reviewed Jun 17, 2025

View reviewed changes

Devesh Kumar Singh added 3 commits June 27, 2025 11:55

Merge remote-tracking branch 'origin/master' into HDDS-12835

743fb32

HDDS-12835. Refactored code as per comments from Tsz.

a181d88

HDDS-12835. Javadoc and faild test case..

092d629

devmadhuu requested a review from szetszwo June 27, 2025 13:22

Devesh Kumar Singh added 4 commits July 7, 2025 17:50

Merge remote-tracking branch 'origin/master' into HDDS-12835

d280c4b

HDDS-12835. Failed test case..

2287261

HDDS-12835. Failed test case..

f88b884

HDDS-12835. Failed test case..

11e2a36

sumitagrawl reviewed Jul 11, 2025

View reviewed changes

devmadhuu closed this Jul 11, 2025

		for (KeyLocationList keyLocationList : keyInfo.getKeyLocationListList()) {
		for (OzoneManagerProtocolProtos.KeyLocationList keyLocationList : keyInfo.getKeyLocationListList()) {

		Table<String, OmDirectoryInfo> dirTable = omMetadataManager.getStore()
		.getTable(DIRECTORY_TABLE, StringCodec.get(), CUSTOM_CODEC_FOR_DIR_TABLE, TableCache.CacheType.NO_CACHE);

Conversation

devmadhuu commented Jun 2, 2025

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Uh oh!

adoroszlai commented Jun 3, 2025

Uh oh!

devmadhuu commented Jun 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devmadhuu commented Jun 27, 2025

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

devmadhuu commented Jul 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adoroszlai Jun 11, 2025 •

edited

Loading