HDDS-6491. Support FSO keys in getExpiredOpenKeys

[kaijchen]

What changes were proposed in this pull request?

Support getting expired FSO keys in OMMetadataManagerImpl#getExpiredOpenKeys() by adding a BucketLayout parameter.

Return List<OpenKeyBucket> instead of List<String>.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6491

How was this patch tested?

Unit test.

[kaijchen]

I have reworked this patch using the List<OpenKeyBucket> approach.
Cc @rakeshadr and @errose28 for review.

[kerneltime]

minor nits around inline path calculations that should be extracted out and tested independently.

[errose28]

Sorry I was late getting to this @kaijchen. Mostly lgtm with a few comments.

[errose28]

Why are we tracking parent ID here, instead of just putting the open key DB name as the name field? This comment made it sound like we were just tracking the name as it appears in the open key/file table. Also I think we would need a field to indicate which table (key or file) this key must be retrieved from.

[kaijchen]

For the same reason, maybe we can get rid of clientID also?

We just need 2 fields to indicate an open key:

1. dbKeyName: /volumeName/bucketName/keyName/clientID or parentID/fileName/clientID.
2. bucketLayout: FILE_SYSTEM_OPTIMIZED or others.

[errose28]

I believe this will work. The bucket layout should be set based on whether the key was retrieved from the open key table or open file table, since the bucket itself may no longer exist. The field will be used by followers to know which table they need to move the key from.

It is interesting that FSO has its own table for open keys, but looks to share the same delete table as OBS and LEGACY keys. I would think there would either be different tables for both or they would both use the same table. Either way, by having this bucket layout field we could handle a delete table unique to FSO in the future. cc @rakeshadr to inquire why the tables are used in this way out of curiosity, although it does not really affect this patch.

[kaijchen]

Since removing a field in proto is hard, I decide not to add BucketLayout for now.
Because the caller must know which bucketLayout it has passed to getExpiredOpenKeys.
Maybe we should add bucketLayout in OpenKeysDeleteRequest instead.

[rakeshadr]

@errose28 Thanks for bringing up this point.

It is interesting that FSO has its own table for open keys, but looks to share the same delete table as OBS and LEGACY keys. I would think there would either be different tables for both or they would both use the same table.

Following are the bucket layout FSO feature specific tables. Since KeyDeletingService uses an offline table which handles deleted files and its physical block cleanup, we thought of making use of the existing logic with minimal efforts for the cleanup and again there was no requirement to introduce a logic based on specific bucket layout types. I am open to introduce a FSO specific table if there is a strong requirement.

* FSO/Prefix Tables:
* |----------------------------------------------------------------------|
* |  directoryTable    | parentId/directoryName -> DirectoryInfo         | 
* |----------------------------------------------------------------------|
* |  fileTable         | parentId/fileName -> KeyInfo                    |
* |----------------------------------------------------------------------|
* |  openFileTable     | parentId/fileName/id -> KeyInfo                 |
* |----------------------------------------------------------------------|
* |  deletedDirTable   | parentId/directoryName -> KeyInfo    
* |----------------------------------------------------------------------|

[kaijchen]

I have decided not to add the BucketLayout in OpenKeyBucket proto in this patch for following reasons:

1. We are processing one kind of bucket at a time in getExpiredOpenKeys, the caller must know the BucketLayout it passed in. And all the expired keys got are in that kind of bucket.
2. Removing a field from proto is hard, it's better to add it in HDDS-4123 if we really need. I think adding BucketLayout in OpenKeysDeleteRequest is another feasible option.

Please take another look @errose28, thanks.

[captainzmc]

+1 The change looks good.

[captainzmc]

Let's merge this and continue remaining tasks of HDDS-4120. Thanks @kaijchen for the patch and thanks @errose28 @rakeshadr @kerneltime for the review.

[kaijchen]

Thanks @errose28 @rakeshadr @kerneltime and @captainzmc for the review.
We can proceed to HDDS-6476 now.