hanishakoneru commented on a change in pull request #2813:
URL: https://github.com/apache/ozone/pull/2813#discussion_r817252582
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java
##########
@@ -254,11 +279,12 @@ protected S3MultipartUploadCompleteResponse
getOmClientResponse(
protected OMClientResponse getOmClientResponse(String multipartKey,
OMResponse.Builder omResponse, String dbMultipartOpenKey,
- OmKeyInfo omKeyInfo, List<OmKeyInfo> unUsedParts) {
+ OmKeyInfo omKeyInfo, List<OmKeyInfo> unUsedParts,
+ OmBucketInfo omBucketInfo, RepeatedOmKeyInfo keysToDelete) {
Review comment:
Can we please rename keysToDelete to oldKeyVersionsToDelete or something
like that for clarity.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequestWithFSO.java
##########
@@ -149,11 +152,29 @@ protected S3MultipartUploadCompleteResponse
getOmClientResponse(
protected OMClientResponse getOmClientResponse(String multipartKey,
OzoneManagerProtocolProtos.OMResponse.Builder omResponse,
String dbMultipartOpenKey, OmKeyInfo omKeyInfo,
- List<OmKeyInfo> unUsedParts) {
+ List<OmKeyInfo> unUsedParts, OmBucketInfo omBucketInfo,
+ RepeatedOmKeyInfo keysToDelete) {
return new S3MultipartUploadCompleteResponseWithFSO(omResponse.build(),
multipartKey, dbMultipartOpenKey, omKeyInfo, unUsedParts,
- getBucketLayout());
+ getBucketLayout(), omBucketInfo, keysToDelete);
+ }
+
+ private long getParentId(OMMetadataManager omMetadataManager,
+ String volumeName, String bucketName, String keyName) throws IOException
{
+
Review comment:
Is there any reason for duplicating getParentId() method here?
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java
##########
@@ -205,9 +210,29 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo keysToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ omMetadataManager, omBucketInfo.getIsVersionEnabled(),
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
Review comment:
We are reading the key twice from the DB. Once here and also in
getOldVersionsToCleanUp().
We can optimize by first reading the key from the DB and getting the older
versions to cleanup only if the key is not null. If the key does not exist in
the DB, we do not need to update the deletedTable at all.
```
OmKeyInfo keyToDelete =
omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
RepeatedOmKeyInfo keysToDelete = getOldVersionsToCleanUp(dbOzoneKey,
omMetadataManager, omBucketInfo.getIsVersionEnabled(),
trxnLogIndex, ozoneManager.isRatisEnabled());
}
```
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java
##########
@@ -205,9 +210,29 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo keysToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ omMetadataManager, omBucketInfo.getIsVersionEnabled(),
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
Review comment:
I was looking into `OMKeyCommitRequest` and
`OMKeyCommmitRequestWithFSO`, and this optimization can be applied there as
well.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/s3/multipart/S3MultipartUploadCompleteResponse.java
##########
@@ -94,19 +102,23 @@ public void addToDBBatch(OMMetadataManager
omMetadataManager,
// 3. Delete unused parts
if (!partsUnusedList.isEmpty()) {
// Add unused parts to deleted key table.
- RepeatedOmKeyInfo repeatedOmKeyInfo = omMetadataManager.getDeletedTable()
- .get(ozoneKey);
- if (repeatedOmKeyInfo == null) {
- repeatedOmKeyInfo = new RepeatedOmKeyInfo(partsUnusedList);
+ if (keysToDelete == null) {
+ keysToDelete = new RepeatedOmKeyInfo(partsUnusedList);
} else {
- for (OmKeyInfo unUsedPart : partsUnusedList) {
- repeatedOmKeyInfo.addOmKeyInfo(unUsedPart);
+ for (OmKeyInfo unusedParts : partsUnusedList) {
+ keysToDelete.addOmKeyInfo(unusedParts);
}
}
-
+ }
+ if (keysToDelete != null) {
omMetadataManager.getDeletedTable().putWithBatch(batchOperation,
- ozoneKey, repeatedOmKeyInfo);
+ ozoneKey, keysToDelete);
}
+
+ // update bucket usedBytes.
+ omMetadataManager.getBucketTable().putWithBatch(batchOperation,
+ omMetadataManager.getBucketKey(omBucketInfo.getVolumeName(),
+ omBucketInfo.getBucketName()), omBucketInfo);
Review comment:
We should only update the Bucket if there were any keys deleted.
Otherwise, this is a redundant operation and should be avoided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
