-
Notifications
You must be signed in to change notification settings - Fork 499
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
HDDS-10650. Delete hsync key info from openFileTable while deleting directory recursively. #6495
Conversation
…irectory recursively.
...src/main/java/org/apache/hadoop/ozone/om/response/key/OMDirectoriesPurgeResponseWithFSO.java
Outdated
Show resolved
Hide resolved
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.
Thanks @ashishkumar50 the fix. Patch looks straightforward to me. This is what #6079 had missed.
lgtm. just one nit above.
Thanks @smengcl for the review, updated above suggestion. |
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.
We should not delete them directly from openKeyTable here. Direct delete will result in blocks not released, causing orphan blocks. We can mark these hsynced open key delectable, and modify openKeyCleanupService to delete these keys and release the blocks.
@ChenSammi Yes It seems the last block may be orphan on DN if we delete directly from table. I think this PR also need to avoid directly deleting and should use openKeyCleanupService @smengcl |
@ChenSammi Good point. A simple solution could be to update
@ashishkumar50 Yes we now need to update |
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.
@ChenSammi Which approach do you suggest, Whether we should use |
@ChenSammi @smengcl Handled using OpenKeyCleanupService. PTAL thanks. |
OmKeyInfo openKeyInfo = omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey); | ||
if (openKeyInfo != null) { | ||
openKeyInfo.getMetadata().put(DELETED_HSYNC_KEY, "true"); |
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.
But what happens when the same client tries to create a key at the same path?
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.
Tell me if this would happen:
- When the same client (with the same client ID, thus same
dbOpenKey
) is allowed to write to the same path, theopenKeyTable
(openFileTable
) entry will be overwritten, still leading to orphan blocks.
On the other hand, it also doesn't make sense to block the same client from writing to the same key path, just because the key is deleted by another client or because the key is errorneously deleted without being close()d first.
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.
I think this problem exist even for non hsync
keys as well.
When Client1 is writing a key, and some other client comes and deletes the directory or bucket and recreate again. Client1 will fail to commit the key and also will not able to write same key till openKey
is cleaned up or it may overwrite.
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.
will not able to write same key till
openKey
is cleaned up
Got it. So at least in this edge case it won't cause new orphan blocks. But we should revisit if this is the desired behavior.
Do we have CLI command to manually trigger a run of OpenKeyCleanup
?
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.
I checked but don't find any CLI to manually trigger OpenKeyCleanup.
import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.OK; | ||
import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.PARTIAL_DELETE; | ||
|
||
/** | ||
* Response for DeleteKey request. | ||
*/ | ||
@CleanupTableInfo(cleanupTables = {KEY_TABLE, OPEN_KEY_TABLE, DELETED_TABLE, BUCKET_TABLE}) | ||
@CleanupTableInfo(cleanupTables = {KEY_TABLE, DELETED_TABLE, BUCKET_TABLE}) |
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.
Why OPEN_KEY_TABLE is removed here? Could both OPEN_KEY_TABLE and OPEN_FILE_TABLE included here, and other involved response classes
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.
Now we are not deleting from OPEN_KEY_TABLE
here which was added in #6079. Instead using OpenKeyCleanupService
to do the same. Still it is required?
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.
This @CleanupTableInfo is to cleanup the table cache after the corresponding transactions flushed to RocksDB. In the delete, we put the empty entry in openKeyTable cache to represent key is deleted from the RocksDB.
// Remove the open key by putting a tombstone entry
openKeyTable.addCacheEntry(dbOpenKey, trxnLogIndex);
But now we alter the key metadata instead of delete the key. So shall we still put an empty entry here? This also reminds me we may need check the output of CLI command which list the openKeys, we need to tell if the key is marked for deleted or not.
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.
Removed adding empty cache entry here.
Okay I will raise Jira to update CLI for displaying whether openKey
is marked for deletion or not.
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.
@ashishkumar50 , I don't mean remove the cache entry adding, instead I think we should add the cache entry with update the keyInfo, instead of an empty entry currently.
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.
Added with updated keyInfo.
@@ -180,7 +185,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn | |||
omClientResponse = new OMKeyDeleteResponse( | |||
omResponse.setDeleteKeyResponse(DeleteKeyResponse.newBuilder()) | |||
.build(), omKeyInfo, ozoneManager.isRatisEnabled(), | |||
omBucketInfo.copyObject(), dbOpenKey); | |||
omBucketInfo.copyObject(), openKeyInfoMap); |
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.
Please pass openKeyInfo instead of a Map here, and below OMKeyDeleteRequestWithFSO.java.
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.
Done
if (!openKeyInfoMap.isEmpty()) { | ||
for (Map.Entry<String, OmKeyInfo> entry : openKeyInfoMap.entrySet()) { | ||
omMetadataManager.getOpenKeyTable(getBucketLayout()).putWithBatch( | ||
batchOperation, entry.getKey(), entry.getValue()); |
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.
@ashishkumar50 , since the file is kept in openKeyTable for lazy deletion, we shall also need to check this "DELETED_HSYNC_KEY" flag in allocateBlock, commitKey and recoverLease request.
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.
Done
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.
The last patch LGTM. Thanks @ashishkumar50 .
@smengcl , would you like to take another look?
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.
Thanks @ashishkumar50 . The latest revision lgtm
Hi @ashishkumar50 , do we need to cherry-pick this to |
…irectory recursively. (apache#6495)
…irectory recursively. (apache#6495)
…irectory recursively. (apache#6495) (cherry picked from commit fd188d1)
What changes were proposed in this pull request?
When volume or bucket or directory is deleted recursively and if corresponding hsync keys are present in openFileTable, then we should delete those keys as well.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10650
How was this patch tested?
New integration test