HDDS-8463. S3 key uniqueness in deletedTable

[sumitagrawl]

What changes were proposed in this pull request?

for multipart, delete key is multi-part + objectId, where multi-part format is /volumeName/bucketName/keyname/uploadId

What is the link to the Apache JIRA

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

How was this patch tested?

Tested with UT and integration cased present

[sumitagrawl]

@szetszwo plz review

[szetszwo]

@sumitagrawl , we should fix HDDS-8550 first.

[sumitagrawl]

@sumitagrawl , we should fix HDDS-8550 first.

It’s fixed and PR raised

[szetszwo]

@sumitagrawl , thanks a lot for working on this! Please see the comments inlined.

[szetszwo]

After this, the repeatedOmKeyInfo parameter of prepareKeyForDelete(..) is always null. Let's remove it.

+++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java
@@ -446,8 +446,8 @@ public static File createOMDir(String dirPath) {
    * create a new instance to include this key, else we update the existing
    * repeatedOmKeyInfo instance.
    * 3. Set the updateID to the transactionLogIndex.
    * @param keyInfo args supplied by client
-   * @param repeatedOmKeyInfo key details from deletedTable
    * @param trxnLogIndex For Multipart keys, this is the transactionLogIndex
    *                     of the MultipartUploadAbort request which needs to
    *                     be set as the updateID of the partKeyInfos.
@@ -456,8 +456,7 @@ public static File createOMDir(String dirPath) {
    * @return {@link RepeatedOmKeyInfo}
    */
   public static RepeatedOmKeyInfo prepareKeyForDelete(OmKeyInfo keyInfo,
-      RepeatedOmKeyInfo repeatedOmKeyInfo, long trxnLogIndex,
-      boolean isRatisEnabled) {
+      long trxnLogIndex, boolean isRatisEnabled) {
     // If this key is in a GDPR enforced bucket, then before moving
     // KeyInfo to deletedTable, remove the GDPR related metadata and
     // FileEncryptionInfo from KeyInfo.
@@ -473,15 +472,7 @@ public static RepeatedOmKeyInfo prepareKeyForDelete(OmKeyInfo keyInfo,
     // Set the updateID
     keyInfo.setUpdateID(trxnLogIndex, isRatisEnabled);
 
-    if (repeatedOmKeyInfo == null) {
-      //The key doesn't exist in deletedTable, so create a new instance.
-      repeatedOmKeyInfo = new RepeatedOmKeyInfo(keyInfo);
-    } else {
-      //The key exists in deletedTable, so update existing instance.
-      repeatedOmKeyInfo.addOmKeyInfo(keyInfo);
-    }
-
-    return repeatedOmKeyInfo;
+    return new RepeatedOmKeyInfo(keyInfo);
   }

[szetszwo]

It should have partNumber/partName. Object id is the same for all parts.

BTW, let's add a utility method to create multipart delete key.

[sumitagrawl]

@szetszwo Each part is created with new object ID as independent upload, and combined to multipart table in repeatedomkeyinfo. ObjectId is not replaced in keyInfo for the part and remains unique.

but part number / name is not unique as user can do re-upload with same part number. If same part number, it will overwrite. And this can cause duplicate entry in deletedTable. So this is not unique.

[szetszwo]

@sumitagrawl , thanks for the info. Do we have a unit test showing it? If not, could you add one?

[sumitagrawl]

@szetszwo This is already covered by existing testcase and same is updated with objectId as correction as part of this PR,
https://github.com/apache/ozone/pull/4660/files#diff-c1e307469ed5f710651ae2ec86baed4d9dce85d742c1c282cb1e7d28a776b503

As part of this, 2 parts are deleted with unique key: part1DeletedKeyName and part2DeletedKeyName

[szetszwo]

@sumitagrawl , I am asking a unit test. If there is one, which one? I want to run it locally. Thanks.

[sumitagrawl]

@szetszwo TestS3MultipartUploadAbortResponse:testAddDBToBatchWithParts cover the scenario as discussed.

[szetszwo]

@sumitagrawl , tried the testAddDBToBatchWithParts but it is not a real test -- in createPartKeyInfo, it creates parts random an object id but not the id generated by OM.

We should have a test that a real client uploads multipart to OM and then the parts get deleted.

[szetszwo]

... in createPartKeyInfo, it creates parts random an object id but not the id generated by OM.

Indeed, the random ids could be the same since randomness does not prevent collision.

[szetszwo]

I actually have a Multipart Upload test and I am able to verify that the object ids for each part are different.

Batch-61: deletedTable put(key: 98 B, value: 542 B), #put=1, batchSize=640 B, discarded: 0 (0 B); key=/s3v/s3bucket/testKey/76d7cb94-3369-470c-a83f-28448135f131-110365366127820801/-9223372036854771968
Batch-61: deletedTable put(key: 98 B, value: 542 B), #put=2, batchSize=1.25 KB, discarded: 0 (0 B); key=/s3v/s3bucket/testKey/76d7cb94-3369-470c-a83f-28448135f131-110365366127820801/-9223372036854771712
Batch-61: deletedTable put(key: 98 B, value: 542 B), #put=3, batchSize=1.88 KB, discarded: 0 (0 B); key=/s3v/s3bucket/testKey/76d7cb94-3369-470c-a83f-28448135f131-110365366127820801/-9223372036854770688
Batch-61: deletedTable put(key: 98 B, value: 542 B), #put=4, batchSize=2.50 KB, discarded: 0 (0 B); key=/s3v/s3bucket/testKey/76d7cb94-3369-470c-a83f-28448135f131-110365366127820801/-9223372036854770432
Batch-61: deletedTable put(key: 98 B, value: 542 B), #put=5, batchSize=3.13 KB, discarded: 0 (0 B); key=/s3v/s3bucket/testKey/76d7cb94-3369-470c-a83f-28448135f131-110365366127820801/-9223372036854771456
Batch-61: deletedTable put(key: 98 B, value: 542 B), #put=6, batchSize=3.75 KB, discarded: 0 (0 B); key=/s3v/s3bucket/testKey/76d7cb94-3369-470c-a83f-28448135f131-110365366127820801/-9223372036854771200
Batch-61: deletedTable put(key: 98 B, value: 542 B), #put=7, batchSize=4.38 KB, discarded: 0 (0 B); key=/s3v/s3bucket/testKey/76d7cb94-3369-470c-a83f-28448135f131-110365366127820801/-9223372036854772992
Batch-61: deletedTable put(key: 98 B, value: 542 B), #put=8, batchSize=5 KB, discarded: 0 (0 B); key=/s3v/s3bucket/testKey/76d7cb94-3369-470c-a83f-28448135f131-110365366127820801/-9223372036854772224
Batch-61: deletedTable put(key: 98 B, value: 542 B), #put=9, batchSize=5.63 KB, discarded: 0 (0 B); key=/s3v/s3bucket/testKey/76d7cb94-3369-470c-a83f-28448135f131-110365366127820801/-9223372036854770176
Batch-61: deletedTable put(key: 98 B, value: 542 B), #put=10, batchSize=6.25 KB, discarded: 0 (0 B); key=/s3v/s3bucket/testKey/76d7cb94-3369-470c-a83f-28448135f131-110365366127820801/-9223372036854772736
  Batch-61: deletedTable batchSize=6.25 KB, discarded: 0 (0 B), #put=10, #del=0

[szetszwo]

+1 the change looks good.