HDDS-4266: CreateFile : store parent dir entries into DirTable and file entry into separate FileTable

[rakeshadr]

What changes were proposed in this pull request?

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

What is the link to the Apache JIRA

This task is to handle the createFileRequest. OM will store all the missing parent dirs to DirTable and leaf file node into a separate FileTable. This new FileTable and OpenFileTable is basically to segregate keys and files.

How was this patch tested?

Added UTs - TestOMFileCreateRequestV1, TestOMKeyCommitRequestV1, TestOzoneFileOps.

[linyiqun]

@rakeshadr , thanks for working on this. Initial review comments for this.
Haven't finished the review, will look deep on this.

[linyiqun]

Below are the review comments for the unit tests.

[rakeshadr]

Thanks a lot @linyiqun for the review comments and I have updated PR addressing the same. Please take another look at it when you get a chance.

[bharatviswa504]

Looks like in new version we don't need to store full keyName in OmKeyInfo, but we still store it, as OMKeyInfoCodec has still set Keyname and convert to proto.

 public KeyInfo getProtobuf(boolean ignorePipeline) {
    long latestVersion = keyLocationVersions.size() == 0 ? -1 :
        keyLocationVersions.get(keyLocationVersions.size() - 1).getVersion();

    List<KeyLocationList> keyLocations = new ArrayList<>();
    for (OmKeyLocationInfoGroup locationInfoGroup : keyLocationVersions) {
      keyLocations.add(locationInfoGroup.getProtobuf(ignorePipeline));
    }

    KeyInfo.Builder kb = KeyInfo.newBuilder()
        .setVolumeName(volumeName)
        .setBucketName(bucketName)
        .setKeyName(keyName)

[bharatviswa504]

Why can't we use keyName as leaf node name for new requests, so that we don't need to store full pathName for OmKeyInfo for new request processing.

[rakeshadr]

OMKeyInfo is exposed to Ozone client, so I have kept 'keyName' as it is. Anyway fileName can be extracted from the keyName so that I thought of reducing the load on DB.

Hope this make sense to you.

[bharatviswa504]

But now OmKeyInfo has entire keyPath which is not really needed to be persisted to DB in this new V1 Request.

[rakeshadr]

I completely agree with you that, V1 need only the fileName in DB.

On a second thought, do we need to maintain two different OMKeyInfo#keyName semantics in DB. For 100% S3 , 'keyName' stores full path. For 100% FS, 'keyName' stores only the fileName in the value part.

message keyInfo {
       ....
       required string keyName = 3;

[rakeshadr]

Done the changes in new commit

[bharatviswa504]

Added to the cache correctly(With KeyName as leaf node name), but in response class, we passed omKeyInfo this should be also the same thing which we have added to the cache right?

[rakeshadr]

Inside OMFileCreateResponseV1#addToDBBatch() method has the actual call to the DB, it is doing the changes in utility OMFileRequest function. I have done this way to ensure that, only when updating the TableCache and persisting to the Table it makes the DB representation as keyName=leafNodeName. All the other places(any functional checks inside OM, logging inside OM, client response etc..) will have the keyName=fullPath and fileName=leafNodeName.

OMFileRequest.addToOpenFileTable(omMetadataMgr, batchOp, getOmKeyInfo(),
         getOpenKeySessionID());

Also, for better code maintenance, I will be adding these conversion in OMFileRequest utility functions. Hope this is fine for you?

[bharatviswa504]

With the current approach in PR looks like we do unnecessarily one more time copy object. What we add to the cache, if we pass the same instance to Response class then same can be persisted (Even for this, we can follow current model, same instance is in cache and Response classes, and get always return copy object). With this way cache and Response classes uses the same instance, and anyway in logging in createRequest we use Keyname, I see no issue over there.

[rakeshadr]

Sure, updated patch by addressing this point. Thanks!

[linyiqun]

Thanks for addressing the comments, @rakeshadr . Few other comments from me.
Also please have a fix on the checkstyle warning.

[rakeshadr]

Thanks a lot @bharatviswa504 and @linyiqun for the review efforts and useful comments. I've made another commit by addressing your comments.

[linyiqun]

@rakeshadr , thanks for addressing all the comments!

[rakeshadr]

@linyiqun I've resolved the comments which are fixed. Please feel free to re-open, if anything extra to be handled.

[linyiqun]

Please see my latest comment, @rakeshadr .
Other places seem good to me now.

[rakeshadr]

Thank you @linyiqun and @bharatviswa504 for the continuous help in reviews. Please let me know if any more comments.

[rakeshadr]

Thank you @linyiqun and @bharatviswa504 for the continuous help in reviews. Please let me know if any more comments.

@linyiqun, @bharatviswa504 Can you please approve the PR if there are no more comments. Thanks again!

[rakeshadr]

Thank You for the updated patch.
I have one comment, (resolved fixed comments), rest LGTM.

Thanks @bharatviswa504 for the detailed reviews. I've updated the patch addressing the comment.

[bharatviswa504]

+1 LGTM
Thank You @rakeshadr for the contribution and for continuously updating the patch patiently and @linyiqun for the review.