You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sumitagrawl (via GitHub)" <gi...@apache.org> on 2023/04/21 03:24:45 UTC
[GitHub] [ozone] sumitagrawl opened a new pull request, #4602: HDDS-8462. Key in deleteTable uniqueness
sumitagrawl opened a new pull request, #4602:
URL: https://github.com/apache/ozone/pull/4602
## What changes were proposed in this pull request?
1. delete key is appended with objectId to make it unique
2. Removed caching of deleteTable as its not used again due to uniqueness
and removed getting from deleteTable for old key (old key will not be present as delete key is unique)
3. This is to add repeated info and overwrite the same key again in deleteTable, which can cause missing cleanup of latest block appended as there KeyDeletingService has already run in parallel.
Uniqueness for multiple flows:
openKey cleanup: openKey appended with objectId
key commit(unused block and old version): openKey appended with objectId
delete key/keys: dbKey appended with objectId
directoryPurge: key appended with objectId
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8462
## How was this patch tested?
Unit test and integration test for delete behavior impact
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1184597836
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMTrashRecoverResponse.java:
##########
@@ -55,8 +55,10 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
String trashKey = omMetadataManager
.getOzoneKey(omKeyInfo.getVolumeName(),
omKeyInfo.getBucketName(), omKeyInfo.getKeyName());
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ omKeyInfo.getObjectID(), trashKey);
RepeatedOmKeyInfo repeatedOmKeyInfo = omMetadataManager
- .getDeletedTable().get(trashKey);
+ .getDeletedTable().get(deleteKey);
Review Comment:
@szetszwo This code is unused and needs to be removed, a JIRA [HDDS-8531](https://issues.apache.org/jira/browse/HDDS-8531) is created by @sadanand48 for handling same.
This does not have any problem.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1185249649
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMTrashRecoverResponse.java:
##########
@@ -55,8 +55,10 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
String trashKey = omMetadataManager
.getOzoneKey(omKeyInfo.getVolumeName(),
omKeyInfo.getBucketName(), omKeyInfo.getKeyName());
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ omKeyInfo.getObjectID(), trashKey);
RepeatedOmKeyInfo repeatedOmKeyInfo = omMetadataManager
- .getDeletedTable().get(trashKey);
+ .getDeletedTable().get(deleteKey);
Review Comment:
For Trash Recovery unused code, just changed based on change in format all the places, to have same format.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1181882746
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -247,11 +251,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OmKeyInfo pseudoKeyInfo = wrapUncommittedBlocksAsPseudoKey(uncommitted,
omKeyInfo);
if (pseudoKeyInfo != null) {
- if (oldKeyVersionsToDelete != null) {
- oldKeyVersionsToDelete.addOmKeyInfo(pseudoKeyInfo);
- } else {
- oldKeyVersionsToDelete = new RepeatedOmKeyInfo(pseudoKeyInfo);
- }
+ String delKeyName = omMetadataManager.getOzoneDeletePathKey(
+ trxnLogIndex, dbOzoneKey);
Review Comment:
We probably should not pass `trxnLogIndex` as an object id. What if it collides with an existing object id?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -195,7 +195,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
// 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 oldKeyVersionsToDelete = null;
+ Map<String, RepeatedOmKeyInfo> oldKeyVersionsToDeleteMap
+ = new HashMap<>();
Review Comment:
We should create a new HashMap only when there is a `keyToDelete`. Otherwise, it will create a lot of empty HashMap objects.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #4602:
URL: https://github.com/apache/ozone/pull/4602#issuecomment-1535093401
@sumitagrawl , thanks again for working on this!
@aswinshakil , thanks for reviewing this!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1183179723
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java:
##########
@@ -216,16 +230,11 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMFileRequest.addFileTableCacheEntry(omMetadataManager, dbFileKey,
omKeyInfo, fileName, trxnLogIndex);
- if (oldKeyVersionsToDelete != null) {
- OMFileRequest.addDeletedTableCacheEntry(omMetadataManager, dbFileKey,
- oldKeyVersionsToDelete, trxnLogIndex);
- }
Review Comment:
No, this is not required as, earlier, there is possibility for append entries as key name was same. Now key name is unique, so do not have this scenario.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #4602:
URL: https://github.com/apache/ozone/pull/4602#issuecomment-1535029814
There is an error in https://github.com/apache/ozone/actions/runs/4867542164/jobs/8701924461?pr=4602#step:6:791 . Let's rerun the job.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1185269938
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMTrashRecoverResponse.java:
##########
@@ -55,8 +55,10 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
String trashKey = omMetadataManager
.getOzoneKey(omKeyInfo.getVolumeName(),
omKeyInfo.getBucketName(), omKeyInfo.getKeyName());
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ omKeyInfo.getObjectID(), trashKey);
RepeatedOmKeyInfo repeatedOmKeyInfo = omMetadataManager
- .getDeletedTable().get(trashKey);
+ .getDeletedTable().get(deleteKey);
Review Comment:
No. If the code is going to be removed. Please don't change it. We may add a TODO comment to mention that it is going to be removed. It will save everyone's time.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1178220601
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java:
##########
@@ -118,9 +118,12 @@ public RepeatedOmKeyInfo getKeysToDelete() {
protected void updateDeletedTable(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException {
- if (this.keysToDelete != null) {
+ if (this.keysToDelete != null
+ && this.keysToDelete.getOmKeyInfoList().size() > 0) {
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ keysToDelete.getOmKeyInfoList().get(0).getObjectID(), openKeyName);
Review Comment:
If we use the object id of the first key to represent a list of keys, would it be possible that the same object is deleted again and the second delete overwrites the entry in deletedTable with a single key? Then, it could lose the other keys in the list.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1184529569
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMTrashRecoverResponse.java:
##########
@@ -55,8 +55,10 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
String trashKey = omMetadataManager
.getOzoneKey(omKeyInfo.getVolumeName(),
omKeyInfo.getBucketName(), omKeyInfo.getKeyName());
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ omKeyInfo.getObjectID(), trashKey);
RepeatedOmKeyInfo repeatedOmKeyInfo = omMetadataManager
- .getDeletedTable().get(trashKey);
+ .getDeletedTable().get(deleteKey);
Review Comment:
@sumitagrawl , It seems that `TrashRecover` won't work if there are old keys (without object id) in the deletedTable?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1183183473
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java:
##########
@@ -808,13 +808,7 @@ protected RepeatedOmKeyInfo getOldVersionsToCleanUp(
@Nonnull String dbOzoneKey, @Nonnull OmKeyInfo keyToDelete,
OMMetadataManager omMetadataManager, long trxnLogIndex,
Review Comment:
@szetszwo Updated...
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #4602:
URL: https://github.com/apache/ozone/pull/4602#issuecomment-1535087499
> Now with this change RepeatedOmKeyInfo will only have only entry right? ...
> .., we can not remove handling list as old version might be present having multiple entry. So still need to keep it same currently for compatibility.
@aswinshakil , @sumitagrawl , We may consider having a new table, say `DeletedTableWithObjectId`. Then there is no compatibility issue.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1180600519
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/AbstractOMKeyDeleteResponse.java:
##########
@@ -115,15 +115,15 @@ protected void addDeletionToBatch(
* file table (which is in prefix format) and adds the fullKey
* into the deletedTable
* @param keyName (format: objectId/key)
- * @param fullKeyName (format: vol/buck/key)
+ * @param deleteKeyName (format: vol/buck/key)
Review Comment:
```suggestion
* @param deleteKeyName (format: vol/buck/key/objectId)
```
Please also update `OmMetadataManagerImpl` with the schema change.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1182809861
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java:
##########
@@ -808,13 +808,7 @@ protected RepeatedOmKeyInfo getOldVersionsToCleanUp(
@Nonnull String dbOzoneKey, @Nonnull OmKeyInfo keyToDelete,
OMMetadataManager omMetadataManager, long trxnLogIndex,
Review Comment:
`dbOzoneKey` and `omMetadataManager` are unused. Let's remove them.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java:
##########
@@ -216,16 +230,11 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMFileRequest.addFileTableCacheEntry(omMetadataManager, dbFileKey,
omKeyInfo, fileName, trxnLogIndex);
- if (oldKeyVersionsToDelete != null) {
- OMFileRequest.addDeletedTableCacheEntry(omMetadataManager, dbFileKey,
- oldKeyVersionsToDelete, trxnLogIndex);
- }
Review Comment:
When `oldKeyVersionsToDeleteMap != null`, should we keep adding the entries to cache?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1180540098
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java:
##########
@@ -118,9 +118,12 @@ public RepeatedOmKeyInfo getKeysToDelete() {
protected void updateDeletedTable(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException {
- if (this.keysToDelete != null) {
+ if (this.keysToDelete != null
+ && this.keysToDelete.getOmKeyInfoList().size() > 0) {
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ keysToDelete.getOmKeyInfoList().get(0).getObjectID(), openKeyName);
Review Comment:
First of all, I disagree that we can use the first key of the list (i,e, `getOmKeyInfoList().get(0)`) to represent the entire list. Why not putting a deleteKey for each element in the list?
> ... one issue will be handled as part of [HDDS-8467](https://issues.apache.org/jira/browse/HDDS-8467)
As mentioned previously, we cannot introduce bugs to the master branch and fix it later.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #4602:
URL: https://github.com/apache/ozone/pull/4602#issuecomment-1532360965
> The changes mostly look good to me. I have a few questions,
>
> 1. How does this handle upgrades? I guess older entries in the `deletedTable` before the upgrade will still be cleaned up by `KeyDeletingService`, while the newer entries after the upgrade use this new flow. If that's the case can we remove `getOldVersionsToCleanUp()` because there will be no older version with this new key format `/volume/bucket/key/objID`
> 2. Now with this change `RepeatedOmKeyInfo` will only have only entry right? As a part of Snapshot GC, we split the `RepeatedOmKeyInfo` into reclaimable keys and non-reclaimable keys. If it only has one entry we can avoid that step altogether.
@aswinshakil
1. Related to upgrade, there is no impact to old entry already present in deletedTable as adding objectId is making it unique for any further addition of deleted key. Processing of deleted key will remain same for old version format and new format.
2. For Snapshot GC, we can not remove handling list as old version might be present having multiple entry. So still need to keep it same currently for compatibility.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo merged pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #4602:
URL: https://github.com/apache/ozone/pull/4602
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1181588746
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java:
##########
@@ -118,9 +118,12 @@ public RepeatedOmKeyInfo getKeysToDelete() {
protected void updateDeletedTable(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException {
- if (this.keysToDelete != null) {
+ if (this.keysToDelete != null
+ && this.keysToDelete.getOmKeyInfoList().size() > 0) {
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ keysToDelete.getOmKeyInfoList().get(0).getObjectID(), openKeyName);
Review Comment:
@szetszwo @aswinshakil Updated... For old version, using obectId of old version, for uncommited block using trxLogIndex.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/AbstractOMKeyDeleteResponse.java:
##########
@@ -115,15 +115,15 @@ protected void addDeletionToBatch(
* file table (which is in prefix format) and adds the fullKey
* into the deletedTable
* @param keyName (format: objectId/key)
- * @param fullKeyName (format: vol/buck/key)
+ * @param deleteKeyName (format: vol/buck/key)
Review Comment:
Updated...
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1182767560
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -195,7 +195,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
// 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 oldKeyVersionsToDelete = null;
+ Map<String, RepeatedOmKeyInfo> oldKeyVersionsToDeleteMap
+ = new HashMap<>();
Review Comment:
Updated...
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -247,11 +251,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OmKeyInfo pseudoKeyInfo = wrapUncommittedBlocksAsPseudoKey(uncommitted,
omKeyInfo);
if (pseudoKeyInfo != null) {
- if (oldKeyVersionsToDelete != null) {
- oldKeyVersionsToDelete.addOmKeyInfo(pseudoKeyInfo);
- } else {
- oldKeyVersionsToDelete = new RepeatedOmKeyInfo(pseudoKeyInfo);
- }
+ String delKeyName = omMetadataManager.getOzoneDeletePathKey(
+ trxnLogIndex, dbOzoneKey);
Review Comment:
Updated...
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1180531435
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java:
##########
@@ -118,9 +118,12 @@ public RepeatedOmKeyInfo getKeysToDelete() {
protected void updateDeletedTable(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException {
- if (this.keysToDelete != null) {
+ if (this.keysToDelete != null
+ && this.keysToDelete.getOmKeyInfoList().size() > 0) {
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ keysToDelete.getOmKeyInfoList().get(0).getObjectID(), openKeyName);
Review Comment:
@szetszwo
In keyList, there will be possible 2 entry as keysToDelete.getOmKeyInfoList(),
- if VersionEnabled is false and old key is present, it will use old key objectId which will be unique.
- If above is not present for delete, and uncommited block is present, it will use OBJECT_ID_RECLAIM_BLOCKS which have value "0", can can have conflict key., This part will be handled as part of [HDDS-8467](https://issues.apache.org/jira/browse/HDDS-8467)
So this currently do not have issue and one issue will be handled as part of HDDS-8467
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1180529382
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java:
##########
@@ -118,9 +118,12 @@ public RepeatedOmKeyInfo getKeysToDelete() {
protected void updateDeletedTable(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException {
- if (this.keysToDelete != null) {
+ if (this.keysToDelete != null
+ && this.keysToDelete.getOmKeyInfoList().size() > 0) {
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ keysToDelete.getOmKeyInfoList().get(0).getObjectID(), openKeyName);
Review Comment:
BTW, if this pull request has some known problem, we cannot merge it to the master branch even there are JIRAs tracking it. We cannot introduce bugs to master branch and fix it later.
If there is a need, we may create a feature branch. Then, we can merge some not-working intermediate code and then fix it before merging the feature branch to master.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #4602:
URL: https://github.com/apache/ozone/pull/4602#issuecomment-1521304351
Parent Issue: [HDDS-5905](https://issues.apache.org/jira/browse/HDDS-5905)
Related PR from which this split is created: https://github.com/apache/ozone/pull/4541
Old PR which is closed: https://github.com/apache/ozone/pull/3225
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1178991537
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java:
##########
@@ -118,9 +118,12 @@ public RepeatedOmKeyInfo getKeysToDelete() {
protected void updateDeletedTable(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException {
- if (this.keysToDelete != null) {
+ if (this.keysToDelete != null
+ && this.keysToDelete.getOmKeyInfoList().size() > 0) {
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ keysToDelete.getOmKeyInfoList().get(0).getObjectID(), openKeyName);
Review Comment:
@szetszwo A tracking JIRA is already raised to handle this separately [HDDS-8467](https://issues.apache.org/jira/browse/HDDS-8467)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1180522414
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java:
##########
@@ -118,9 +118,12 @@ public RepeatedOmKeyInfo getKeysToDelete() {
protected void updateDeletedTable(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException {
- if (this.keysToDelete != null) {
+ if (this.keysToDelete != null
+ && this.keysToDelete.getOmKeyInfoList().size() > 0) {
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ keysToDelete.getOmKeyInfoList().get(0).getObjectID(), openKeyName);
Review Comment:
@sumitagrawl , [HDDS-8467](https://issues.apache.org/jira/browse/HDDS-8467) is for `OBJECT_ID_RECLAIM_BLOCKS`.
But this comment is talking about `getOmKeyInfoList().get(0)`. They are different problems.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1180597213
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java:
##########
@@ -118,9 +118,12 @@ public RepeatedOmKeyInfo getKeysToDelete() {
protected void updateDeletedTable(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException {
- if (this.keysToDelete != null) {
+ if (this.keysToDelete != null
+ && this.keysToDelete.getOmKeyInfoList().size() > 0) {
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ keysToDelete.getOmKeyInfoList().get(0).getObjectID(), openKeyName);
Review Comment:
Agree with @szetszwo. We might need to handle HDDS-8467, before committing this patch.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #4602: HDDS-8462. Key in deleteTable uniqueness
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4602:
URL: https://github.com/apache/ozone/pull/4602#discussion_r1185218310
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMTrashRecoverResponse.java:
##########
@@ -55,8 +55,10 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
String trashKey = omMetadataManager
.getOzoneKey(omKeyInfo.getVolumeName(),
omKeyInfo.getBucketName(), omKeyInfo.getKeyName());
+ String deleteKey = omMetadataManager.getOzoneDeletePathKey(
+ omKeyInfo.getObjectID(), trashKey);
RepeatedOmKeyInfo repeatedOmKeyInfo = omMetadataManager
- .getDeletedTable().get(trashKey);
+ .getDeletedTable().get(deleteKey);
Review Comment:
If the code is unused, why changing it?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org