HDDS-9335. OMKeyDeleteRequest using the wrong bucket layout

[guohao-rosicky]

What changes were proposed in this pull request?

OMKeyDeleteRequest using the wrong bucket layout

What is the link to the Apache JIRA

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

How was this patch tested?

existing UT

[sodonnel]

Looks correct to me, but I'll also let someone who knows more about OM comment.

[hemantk-12]

Change looks good to me.

In testing, you said existing unit tests. I'm curios how it was not caught by existing unit/integration tests. Are we missing tests for it? If yes, please add the relevant tests.

[ivandika3]

Thanks for the patch.

I noticed there are some inconsistencies in the usage of bucket layout in the request.

OmKeyInfo omKeyInfo =
          omMetadataManager.getKeyTable(bucketLayout).get(objectKey);

and

omMetadataManager.getKeyTable(getBucketLayout()).addCacheEntry(
          new CacheKey<>(
              omMetadataManager.getOzoneKey(volumeName, bucketName, keyName)),
          CacheValue.get(trxnLogIndex));

The first one uses the bucketLayout from the method argument, the second one uses getBucketLayout. Maybe we can replace the validateAndUpdateCache with the bucketLayout argument with the one without bucketLayout argument, and uses getBucketLayout instead?

[sumitagrawl]

@guohao-rosicky Thanks for working over this,
bucketLayout used here is only to get respective Table: fileTable or keyTable (based on bucket type), so using DEFAULT or OBS (as provided in constructor) does not have any impact to functionality.

To avoid confusion from code perspective, this changes seems good.

Additionally, IMO, we can remove method with passing bucketLayout and merge both, as below is used only in UT cases and getBucketLayout() itself can be used with no impact.

[guohao-rosicky]

we can remove method with passing bucketLayout and merge both

Thanks @sodonnel @ivandika3 @hemantk-12 @sumitagrawl for the review, I think it's a good idea. I'll modify it.

[sumitagrawl]

@guohao-rosicky Plz fix UT cases as impact of changes

[guohao-rosicky]

@guohao-rosicky Plz fix UT cases as impact of changes

Thanks for the review @sumitagrawl, PTAL.
All ci passed, see: https://github.com/guohao-rosicky/ozone/actions/runs/6438542195

[sumitagrawl]

@guohao-rosicky LGTM +1