bharatviswa504 commented on a change in pull request #2433:
URL: https://github.com/apache/ozone/pull/2433#discussion_r714186268
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java
##########
@@ -442,12 +442,20 @@ public static File createOMDir(String dirPath) {
* of the MultipartUploadAbort request which needs to
* be set as the updateID of the partKeyInfos.
* For regular Key deletes, this value should be set to
- * the same updaeID as is in keyInfo.
+ * the same updateID as is in keyInfo.
* @return {@link RepeatedOmKeyInfo}
*/
public static RepeatedOmKeyInfo prepareKeyForDelete(OmKeyInfo keyInfo,
RepeatedOmKeyInfo repeatedOmKeyInfo, long trxnLogIndex,
boolean isRatisEnabled) {
+
+ // Set the updateID
+ keyInfo.setUpdateID(trxnLogIndex, isRatisEnabled);
+ return prepareKeyForDeleteWithoutUpdateID(keyInfo, repeatedOmKeyInfo);
Review comment:
Not understood the reasoning for split of this method?
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -156,6 +157,9 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager,
volumeName, bucketName);
validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
+ // validateBucketAndVolume guarantees bucket existence and it's not null
+ assert omBucketInfo != null;
Review comment:
Minor NIT: We donot need this. As validateBucketAndVolume throws error
when bucket does not exist.
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java
##########
@@ -286,8 +287,17 @@ public synchronized long addNewVersion(
// it is important that the new version are always at the tail of the
list
OmKeyLocationInfoGroup currentLatestVersion =
keyLocationVersions.get(keyLocationVersions.size() - 1);
+
+ // The new version is created based on the latest version number
+ // and will not include key locations of old versions, until object
Review comment:
This comment is confusing.
The latest version holding previous block versions is removed as part
HDDS-5472. Old versions of location in OmKeyLocationInfoGroup causes OOM of
OM (#2448)
And now also when keepOldVersions is false, we create a new version. (With
only that version blocks)
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponseWithFSO.java
##########
@@ -32,19 +33,21 @@
import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.OPEN_FILE_TABLE;
import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.FILE_TABLE;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.DELETED_TABLE;
/**
* Response for CommitKey request - prefix layout1.
*/
-@CleanupTableInfo(cleanupTables = {OPEN_FILE_TABLE, FILE_TABLE})
+@CleanupTableInfo(cleanupTables = {OPEN_FILE_TABLE, FILE_TABLE, DELETED_TABLE})
public class OMKeyCommitResponseWithFSO extends OMKeyCommitResponse {
public OMKeyCommitResponseWithFSO(@Nonnull OMResponse omResponse,
- @Nonnull OmKeyInfo omKeyInfo,
- String ozoneKeyName, String openKeyName,
- @Nonnull OmBucketInfo omBucketInfo) {
+ @Nonnull OmKeyInfo omKeyInfo,
Review comment:
Indentation is incorrect
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -191,6 +195,21 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
+ // TODO(HDDS-5461): Currently, old versions are all moved to the delete
+ // table. To preserve old version(s) in the key table on overwrite,
+ // merge old versions to omKeyInfo here instead of moving the to the
+ // delete table.
Review comment:
This comment is not clear to me.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java
##########
@@ -143,6 +149,22 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
+ // TODO(HDDS-5461): Currently, old versions are all moved to the delete
+ // table. To preserve old version(s) in the key table on overwrite,
+ // merge old versions to omKeyInfo here instead of moving the to the
+ // delete table.
+ // If bucket versioning is turned on somehow, overwritten blocks will
+ // leak and never collected by block deletion service. Keep it off.
+ RepeatedOmKeyInfo keysToDelete = getOldVersionsToCleanUp(dbFileKey,
+ omMetadataManager, omBucketInfo.getIsVersionEnabled(),
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ if (omMetadataManager.getKeyTable().isExist(dbFileKey)
+ && omBucketInfo.getIsVersionEnabled()) {
+ // TODO: delete this warning after versioning supported.
+ LOG.warn("Overwritten blocks of {}/{}/{} may have leaked.",
+ volumeName, bucketName, keyName);
+ }
+
// Add to cache of open key table and key table.
OMFileRequest.addOpenFileTableCacheEntry(omMetadataManager, dbFileKey,
Review comment:
```
if (keysToDelete != null) {
omMetadataManager.getDeletedTable().addCacheEntry(
new CacheKey<>(dbOzoneKey),
new CacheValue<>(Optional.of(keysToDelete), trxnLogIndex));
}
```
Something like this
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -239,6 +259,40 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
return omClientResponse;
}
+ /**
+ * Prepare key for deletion service on overwrite.
+ *
+ * @param dbOzoneKey key to point to an object in RocksDB
+ * @param omMetadataManager
+ * @param isVersionEnabled
+ * @param trxnLogIndex
+ * @param isRatisEnabled
+ * @return Old keys eligible for deletion.
+ * @throws IOException
+ */
+ protected RepeatedOmKeyInfo getOldVersionsToCleanUp(
+ String dbOzoneKey, OMMetadataManager omMetadataManager,
+ boolean isVersionEnabled, long trxnLogIndex,
+ boolean isRatisEnabled) throws IOException {
+ if (isVersionEnabled) {
+ // Nothing to clean up in case versioning is on.
+ return null;
+ }
+ // Past keys that was deleted but still in deleted table,
+ // waiting for deletion service.
+ RepeatedOmKeyInfo keysToDelete =
+ omMetadataManager.getDeletedTable().getReadCopy(dbOzoneKey);
Review comment:
Here we can use normal get(), as delete entries are not added to cache
at all.
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java
##########
@@ -286,8 +287,17 @@ public synchronized long addNewVersion(
// it is important that the new version are always at the tail of the
list
OmKeyLocationInfoGroup currentLatestVersion =
keyLocationVersions.get(keyLocationVersions.size() - 1);
+
+ // The new version is created based on the latest version number
+ // and will not include key locations of old versions, until object
+ // versioning is supported and enabled.
OmKeyLocationInfoGroup newVersion =
currentLatestVersion.generateNextVersion(newLocationList);
+ if (!keepOldVersions) {
+ // Even though old versions are cleared here, they will be
+ // moved to delete table at the time of key commit
+ keyLocationVersions.clear();
+ }
keyLocationVersions.add(newVersion);
Review comment:
And also when oldVersions is false, when a key is override, the version
of that should be zero right?
(Now with this it is still last version + 1)
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java
##########
@@ -29,28 +30,31 @@
import java.io.IOException;
import javax.annotation.Nonnull;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.DELETED_TABLE;
import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.KEY_TABLE;
import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.OPEN_KEY_TABLE;
/**
* Response for CommitKey request.
*/
-@CleanupTableInfo(cleanupTables = {OPEN_KEY_TABLE, KEY_TABLE})
+@CleanupTableInfo(cleanupTables = {OPEN_KEY_TABLE, KEY_TABLE, DELETED_TABLE})
Review comment:
Do we need to add DELETED_TABLE here?
As we donot add any add any entries to delete table cache in
OMKeyCommitRequest.java
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -619,11 +619,11 @@ public void commitKey(OmKeyArgs args, long clientID)
throws IOException {
//update the block length for each block
keyInfo.updateLocationInfoList(locationInfoList, false);
metadataManager.getStore().move(
- openKey,
- objectKey,
- keyInfo,
- metadataManager.getOpenKeyTable(),
- metadataManager.getKeyTable());
+ openKey,
Review comment:
NIT: Spacing is wrong here
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java
##########
@@ -143,6 +149,22 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
+ // TODO(HDDS-5461): Currently, old versions are all moved to the delete
+ // table. To preserve old version(s) in the key table on overwrite,
+ // merge old versions to omKeyInfo here instead of moving the to the
+ // delete table.
+ // If bucket versioning is turned on somehow, overwritten blocks will
+ // leak and never collected by block deletion service. Keep it off.
+ RepeatedOmKeyInfo keysToDelete = getOldVersionsToCleanUp(dbFileKey,
+ omMetadataManager, omBucketInfo.getIsVersionEnabled(),
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ if (omMetadataManager.getKeyTable().isExist(dbFileKey)
+ && omBucketInfo.getIsVersionEnabled()) {
+ // TODO: delete this warning after versioning supported.
Review comment:
Same as above:
Not sure why this blocks will be leaked, if we move the entire OmKeyInfo to
delete table during delete.
Deletion logic should consider all versions in OmKeyInfo and send those
blocks. (If this is missing we should open a bug for this)
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyLocationInfoGroup.java
##########
@@ -143,16 +143,16 @@ public static OmKeyLocationInfoGroup getFromProtobuf(
}
/**
- * Given a new block location, generate a new version list based upon this
- * one.
+ * Given a new block location, generate a new version list based upon the
+ * version number of this object. It does not keep past blocks.
*
* @param newLocationList a list of new location to be added.
* @return newly generated OmKeyLocationInfoGroup
*/
OmKeyLocationInfoGroup generateNextVersion(
List<OmKeyLocationInfo> newLocationList) {
- Map<Long, List<OmKeyLocationInfo>> newMap =
- new HashMap<>();
+ Map<Long, List<OmKeyLocationInfo>> newMap = new HashMap<>();
Review comment:
Minor: Can we avoid this unnecessary change.
So, that it will be easy to track history.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java
##########
@@ -621,13 +621,20 @@ protected OmKeyInfo prepareFileInfo(
//TODO args.getMetadata
}
if (dbKeyInfo != null) {
- // TODO: Need to be fixed, as when key already exists, we are
- // appending new blocks to existing key.
- // The key already exist, the new blocks will be added as new version
- // when locations.size = 0, the new version will have identical blocks
- // as its previous version
- dbKeyInfo.addNewVersion(locations, false);
- dbKeyInfo.setDataSize(size + dbKeyInfo.getDataSize());
+ // The key already exist, the new blocks will replace old ones
+ // as new versions unless the bucket does not have versioning
+ // turned on. If the versioning is on, the new version will have
Review comment:
If the versioning is on, the new version will have
// identical blocks as its previous version.
This is not correct, this has been fixed already. Now each version has its
own blocks.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -191,6 +195,21 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
+ // TODO(HDDS-5461): Currently, old versions are all moved to the delete
+ // table. To preserve old version(s) in the key table on overwrite,
+ // merge old versions to omKeyInfo here instead of moving the to the
+ // delete table.
+ // If bucket versioning is turned on somehow, overwritten blocks will
+ // leak and never collected by block deletion service. Keep it off.
+ RepeatedOmKeyInfo keysToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ omMetadataManager, omBucketInfo.getIsVersionEnabled(), trxnLogIndex,
+ ozoneManager.isRatisEnabled());
+ if (omMetadataManager.getKeyTable().isExist(dbOzoneKey)
+ && omBucketInfo.getIsVersionEnabled()) {
+ // TODO: delete this warning after versioning supported.
+ LOG.warn("Overwritten blocks of {}/{}/{} may have leaked.",
Review comment:
Not sure why this blocks will be leaked, if we move the entire OmKeyInfo
to delete table.
Deletion logic should consider all versions in OmKeyInfo and send those
blocks. (If this is missing we should open a bug for this)
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java
##########
@@ -94,4 +100,12 @@ protected OmBucketInfo getOmBucketInfo() {
protected String getOzoneKeyName() {
return ozoneKeyName;
}
+
+ protected void maybeUpdateDeletedTable(OMMetadataManager omMetadataManager,
Review comment:
Minor: Can we rename this as updateDeletedTable?
##########
File path:
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java
##########
@@ -275,6 +276,76 @@ public void testValidateAndUpdateCacheWithKeyNotFound()
throws Exception {
Assert.assertNull(omKeyInfo);
}
+ @Test
+ public void testValidateAndUpdateCacheOnOverwrite() throws Exception {
+ testValidateAndUpdateCache();
+
+ // Become a new client and set next version number
+ clientID = Time.now();
+ version += 1;
+
+ OMRequest modifiedOmRequest = doPreExecute(createCommitKeyRequest());
+
+ OMKeyCommitRequest omKeyCommitRequest =
+ getOmKeyCommitRequest(modifiedOmRequest);
+
+ KeyArgs keyArgs = modifiedOmRequest.getCommitKeyRequest().getKeyArgs();
+
+ // Key should be there in key table, as validateAndUpdateCache is called.
+ OmKeyInfo omKeyInfo =
+ omMetadataManager.getKeyTable().get(getOzonePathKey());
+
+ Assert.assertNotNull(omKeyInfo);
+ // Previously committed version
+ Assert.assertEquals(0L,
+ omKeyInfo.getLatestVersionLocations().getVersion());
+
+ // Append new blocks
+ List<OmKeyLocationInfo> allocatedLocationList =
+ keyArgs.getKeyLocationsList().stream()
+ .map(OmKeyLocationInfo::getFromProtobuf)
+ .collect(Collectors.toList());
+ addKeyToOpenKeyTable(allocatedLocationList);
+
+ OMClientResponse omClientResponse =
+ omKeyCommitRequest.validateAndUpdateCache(ozoneManager,
+ 102L, ozoneManagerDoubleBufferHelper);
+
+ Assert.assertEquals(OzoneManagerProtocolProtos.Status.OK,
+ omClientResponse.getOMResponse().getStatus());
+
+ String ozoneKey = getOzonePathKey();
+ // Entry should be deleted from openKey Table.
Review comment:
This entry will not exist at all in openKeyTable right?
As in KeyTable entry is with sessionid appended. And here we are checking
ozone key.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -239,6 +259,40 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
return omClientResponse;
}
+ /**
+ * Prepare key for deletion service on overwrite.
+ *
+ * @param dbOzoneKey key to point to an object in RocksDB
+ * @param omMetadataManager
+ * @param isVersionEnabled
+ * @param trxnLogIndex
+ * @param isRatisEnabled
+ * @return Old keys eligible for deletion.
+ * @throws IOException
+ */
+ protected RepeatedOmKeyInfo getOldVersionsToCleanUp(
+ String dbOzoneKey, OMMetadataManager omMetadataManager,
+ boolean isVersionEnabled, long trxnLogIndex,
+ boolean isRatisEnabled) throws IOException {
+ if (isVersionEnabled) {
+ // Nothing to clean up in case versioning is on.
+ return null;
+ }
+ // Past keys that was deleted but still in deleted table,
+ // waiting for deletion service.
+ RepeatedOmKeyInfo keysToDelete =
+ omMetadataManager.getDeletedTable().getReadCopy(dbOzoneKey);
+ // Current key to be overwritten
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable().getReadCopy(dbOzoneKey);
Review comment:
We should use get(). Because if we use getReadCopy() if it is in cache
it might cause issues. (As if any changes happen to that object later)
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponseWithFSO.java
##########
@@ -32,19 +33,21 @@
import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.OPEN_FILE_TABLE;
import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.FILE_TABLE;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.DELETED_TABLE;
/**
* Response for CommitKey request - prefix layout1.
*/
-@CleanupTableInfo(cleanupTables = {OPEN_FILE_TABLE, FILE_TABLE})
+@CleanupTableInfo(cleanupTables = {OPEN_FILE_TABLE, FILE_TABLE, DELETED_TABLE})
Review comment:
Same as above for DELETED_TABLE
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java
##########
@@ -143,6 +149,22 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
+ // TODO(HDDS-5461): Currently, old versions are all moved to the delete
+ // table. To preserve old version(s) in the key table on overwrite,
+ // merge old versions to omKeyInfo here instead of moving the to the
+ // delete table.
+ // If bucket versioning is turned on somehow, overwritten blocks will
+ // leak and never collected by block deletion service. Keep it off.
+ RepeatedOmKeyInfo keysToDelete = getOldVersionsToCleanUp(dbFileKey,
+ omMetadataManager, omBucketInfo.getIsVersionEnabled(),
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ if (omMetadataManager.getKeyTable().isExist(dbFileKey)
+ && omBucketInfo.getIsVersionEnabled()) {
+ // TODO: delete this warning after versioning supported.
+ LOG.warn("Overwritten blocks of {}/{}/{} may have leaked.",
+ volumeName, bucketName, keyName);
+ }
+
// Add to cache of open key table and key table.
OMFileRequest.addOpenFileTableCacheEntry(omMetadataManager, dbFileKey,
Review comment:
I think how we add keyInfo to cache similarly we need to add delete
entries to delete table cache.
Lets say key created/committed we added to cache not yet committed to DB
Before commit, if key is override even if we call get() we donot get
anything as the entry is not in cache and double buffer flush not yet committed
to DB. So, i think we need to add delete entry also cache.
And leave the entry of DELETED_TABLE in cleanup in Response classes.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -619,11 +619,11 @@ public void commitKey(OmKeyArgs args, long clientID)
throws IOException {
//update the block length for each block
keyInfo.updateLocationInfoList(locationInfoList, false);
metadataManager.getStore().move(
- openKey,
- objectKey,
- keyInfo,
- metadataManager.getOpenKeyTable(),
- metadataManager.getKeyTable());
+ openKey,
Review comment:
And also this is unncessary change, revert it back
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -191,6 +195,21 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
+ // TODO(HDDS-5461): Currently, old versions are all moved to the delete
+ // table. To preserve old version(s) in the key table on overwrite,
+ // merge old versions to omKeyInfo here instead of moving the to the
+ // delete table.
Review comment:
In simple we can mention
If versioning is enabled, if key is override we move the old copy of object
to delete table. (Something like that)
--
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]
