HDDS-10650. Delete hsync key info from openFileTable while deleting directory recursively. #6495

ashishkumar50 · 2024-04-08T13:17:15Z

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

…irectory recursively.

...src/main/java/org/apache/hadoop/ozone/om/response/key/OMDirectoriesPurgeResponseWithFSO.java

smengcl

Thanks @ashishkumar50 the fix. Patch looks straightforward to me. This is what #6079 had missed.

lgtm. just one nit above.

ashishkumar50 · 2024-04-10T04:35:03Z

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.

ChenSammi

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.

ashishkumar50 · 2024-04-10T05:52:23Z

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

smengcl · 2024-04-10T06:55:19Z

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 Good point.

A simple solution could be to update deletedKey with the up-to-date block list from the open key before putting it to deletedTable? If the goal is to prevent orphan blocks that should do it.

OpenKeyCleanup should also work. But one small problem with OpenKeyCleanupService is that it is only triggered every 24 hours by default, rather than KeyDeletingService's 60 seconds. In edge cases we might hear admins complain about why the cluster space is not freed upon key deletion after a few hours.

@ashishkumar50 Yes we now need to update OMKeyDeleteRequest and all its variants to contain the correct block list for omKeyInfo. Or take the OpenKeyCleanup approach.

smengcl

~~Marked as blocker for 1.4.1~~ Previously I mistakenly thought HDDS-9930 (which could lead to orphan blocks in the case Sammi mentioned above) was in 1.4.0. I have removed 1.4.1 as a target version.

ashishkumar50 · 2024-04-10T08:50:31Z

@ChenSammi Which approach do you suggest, Whether we should use OpenKeyCleanupService or through KeyDeletingService?
I think for OpenKeyCleanupService we can add metadata in the openKeyTable as deleted during key deletion and use that in OpenKeyCleanupService to identify whether key is deleted.
For KeyDeletingService approach we need to read blocks from openKeyTable and use that block for deletion from SCM/DN.
And remove those keys from openKeyTable as done in this PR.

Update branch

ashishkumar50 · 2024-04-12T09:29:31Z

@ChenSammi @smengcl Handled using OpenKeyCleanupService. PTAL thanks.

smengcl · 2024-04-12T20:06:38Z

...r/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java

+            OmKeyInfo openKeyInfo = omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
+            if (openKeyInfo != null) {
+              openKeyInfo.getMetadata().put(DELETED_HSYNC_KEY, "true");


But what happens when the same client tries to create a key at the same path?

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, the openKeyTable (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.

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.

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?

I checked but don't find any CLI to manually trigger OpenKeyCleanup.

ChenSammi · 2024-04-15T09:34:04Z

...zone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeysDeleteResponse.java

 * Response for DeleteKey request.
 */
-@CleanupTableInfo(cleanupTables = {KEY_TABLE, OPEN_KEY_TABLE, DELETED_TABLE, BUCKET_TABLE})
+@CleanupTableInfo(cleanupTables = {KEY_TABLE, DELETED_TABLE, BUCKET_TABLE})


Why OPEN_KEY_TABLE is removed here？ Could both OPEN_KEY_TABLE and OPEN_FILE_TABLE included here, and other involved response classes

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?

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.

Removed adding empty cache entry here.
Okay I will raise Jira to update CLI for displaying whether openKey is marked for deletion or not.

@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.

Added with updated keyInfo.

ChenSammi · 2024-04-15T09:50:55Z

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java

          omResponse.setDeleteKeyResponse(DeleteKeyResponse.newBuilder())
              .build(), omKeyInfo, ozoneManager.isRatisEnabled(),
-          omBucketInfo.copyObject(), dbOpenKey);
+          omBucketInfo.copyObject(), openKeyInfoMap);


Please pass openKeyInfo instead of a Map here, and below OMKeyDeleteRequestWithFSO.java.

ChenSammi · 2024-04-15T10:19:08Z

...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyDeleteResponse.java

+    if (!openKeyInfoMap.isEmpty()) {
+      for (Map.Entry<String, OmKeyInfo> entry : openKeyInfoMap.entrySet()) {
+        omMetadataManager.getOpenKeyTable(getBucketLayout()).putWithBatch(
+            batchOperation, entry.getKey(), entry.getValue());


@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.

…e in metadata

ChenSammi

The last patch LGTM. Thanks @ashishkumar50 .

@smengcl , would you like to take another look?

smengcl

Thanks @ashishkumar50 . The latest revision lgtm

smengcl · 2024-04-29T14:35:37Z

Hi @ashishkumar50 , do we need to cherry-pick this to master branch as well?

…irectory recursively. (apache#6495)

…irectory recursively. (apache#6495) (cherry picked from commit fd188d1)

ashishkr200 added 2 commits April 8, 2024 16:24

HDDS-10650. Delete hsync metadata from openFileTable while deleting d…

17cfb14

…irectory recursively.

Fix test case

a839610

ashishkumar50 requested review from ChenSammi, jojochuang and smengcl April 8, 2024 13:17

kerneltime requested a review from errose28 April 8, 2024 16:21

jojochuang added the hbase HBase on Ozone support label Apr 9, 2024

smengcl reviewed Apr 10, 2024

View reviewed changes

...src/main/java/org/apache/hadoop/ozone/om/response/key/OMDirectoriesPurgeResponseWithFSO.java Outdated Show resolved Hide resolved

smengcl approved these changes Apr 10, 2024

View reviewed changes

ashishkr200 added 2 commits April 10, 2024 09:58

Fix review comment

10ecd2e

Fix review comment

b6019b3

ChenSammi requested changes Apr 10, 2024

View reviewed changes

smengcl requested changes Apr 10, 2024

View reviewed changes

jojochuang force-pushed the HDDS-7593 branch from d4314c9 to 8aa8a36 Compare April 11, 2024 22:20

ashishkumar50 and others added 3 commits April 12, 2024 11:37

Merge pull request #11 from ashishkumar50/HDDS-7593

a07c6ae

Update branch

Handle openKey cleanup using openKeyCleanupService

175fb7f

Fix test case

ba340ec

smengcl reviewed Apr 12, 2024

View reviewed changes

ChenSammi reviewed Apr 15, 2024

View reviewed changes

ashishkr200 added 3 commits April 15, 2024 17:43

Fix review comments

71acea4

Remove putting empty CacheEntry while keeping openKey in deleted stat…

3cc5638

…e in metadata

Update cache value with latest openKeyInfo detail

e43c053

jojochuang requested a review from ChenSammi April 18, 2024 07:48

jojochuang requested a review from smengcl April 18, 2024 07:48

Clear cache during response processing

90c8672

ChenSammi approved these changes Apr 22, 2024

View reviewed changes

smengcl approved these changes Apr 23, 2024

View reviewed changes

ChenSammi merged commit fd188d1 into apache:HDDS-7593 Apr 24, 2024

chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024

HDDS-10650. Delete hsync key info from openFileTable while deleting d…

063022c

…irectory recursively. (apache#6495)

chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024

HDDS-10650. Delete hsync key info from openFileTable while deleting d…

7556313

…irectory recursively. (apache#6495)

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request May 29, 2024

HDDS-10650. Delete hsync key info from openFileTable while deleting d…

a8a6736

…irectory recursively. (apache#6495) (cherry picked from commit fd188d1)

HDDS-10650. Delete hsync key info from openFileTable while deleting directory recursively. #6495

HDDS-10650. Delete hsync key info from openFileTable while deleting directory recursively. #6495

Uh oh!

Conversation

ashishkumar50 commented Apr 8, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Uh oh!

smengcl left a comment

Choose a reason for hiding this comment

Uh oh!

ashishkumar50 commented Apr 10, 2024

Uh oh!

ChenSammi left a comment

Choose a reason for hiding this comment

Uh oh!

ashishkumar50 commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smengcl commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smengcl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashishkumar50 commented Apr 10, 2024

Uh oh!

ashishkumar50 commented Apr 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi left a comment

Choose a reason for hiding this comment

Uh oh!

smengcl left a comment

Choose a reason for hiding this comment

Uh oh!

smengcl commented Apr 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ashishkumar50 commented Apr 10, 2024 •

edited

Loading

smengcl commented Apr 10, 2024 •

edited

Loading

smengcl left a comment •

edited

Loading

ChenSammi Apr 15, 2024 •

edited

Loading