-
Notifications
You must be signed in to change notification settings - Fork 587
HDDS-8463. S3 key uniqueness in deletedTable #4660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,19 +97,20 @@ public void addToDBBatch(OMMetadataManager omMetadataManager, | |
| OmKeyInfo currentKeyPartInfo = | ||
| OmKeyInfo.getFromProtobuf(partKeyInfo.getPartKeyInfo()); | ||
|
|
||
| RepeatedOmKeyInfo repeatedOmKeyInfo = | ||
| omMetadataManager.getDeletedTable().get(partKeyInfo.getPartName()); | ||
|
|
||
| repeatedOmKeyInfo = OmUtils.prepareKeyForDelete(currentKeyPartInfo, | ||
| repeatedOmKeyInfo, omMultipartKeyInfo.getUpdateID(), isRatisEnabled); | ||
| RepeatedOmKeyInfo repeatedOmKeyInfo = OmUtils.prepareKeyForDelete( | ||
| currentKeyPartInfo, null, omMultipartKeyInfo.getUpdateID(), | ||
| isRatisEnabled); | ||
| // multi-part key format is volumeName/bucketName/keyName/uploadId | ||
| String deleteKey = omMetadataManager.getOzoneDeletePathKey( | ||
| currentKeyPartInfo.getObjectID(), multipartKey); | ||
|
Comment on lines
+104
to
+105
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should have BTW, let's add a utility method to create multipart delete key.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sumitagrawl , thanks for the info. Do we have a unit test showing it? If not, could you add one?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @szetszwo This is already covered by existing testcase and same is updated with objectId as correction as part of this PR, As part of this, 2 parts are deleted with unique key: part1DeletedKeyName and part2DeletedKeyName
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sumitagrawl , I am asking a unit test. If there is one, which one? I want to run it locally. Thanks.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @szetszwo TestS3MultipartUploadAbortResponse:testAddDBToBatchWithParts cover the scenario as discussed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sumitagrawl , tried the We should have a test that a real client uploads multipart to OM and then the parts get deleted.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Indeed, the random ids could be the same since randomness does not prevent collision. |
||
|
|
||
| omMetadataManager.getDeletedTable().putWithBatch(batchOperation, | ||
| partKeyInfo.getPartName(), repeatedOmKeyInfo); | ||
|
|
||
| // update bucket usedBytes. | ||
| omMetadataManager.getBucketTable().putWithBatch(batchOperation, | ||
| omMetadataManager.getBucketKey(omBucketInfo.getVolumeName(), | ||
| omBucketInfo.getBucketName()), omBucketInfo); | ||
| deleteKey, repeatedOmKeyInfo); | ||
| } | ||
|
|
||
| // update bucket usedBytes. | ||
| omMetadataManager.getBucketTable().putWithBatch(batchOperation, | ||
| omMetadataManager.getBucketKey(omBucketInfo.getVolumeName(), | ||
| omBucketInfo.getBucketName()), omBucketInfo); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this, the
repeatedOmKeyInfoparameter ofprepareKeyForDelete(..)is always null. Let's remove it.