HDDS-2949: mkdir : store directory entries in a separate table

[rakeshadr]

What changes were proposed in this pull request?

All the directories from the path components should be created on 'DirectoryTable'. Here, it uses new classes and retained existing logic. This is required during rolling upgrade. So, created V1 classes for the implementation. Later, during rolling upgrade will rebase this feature branch accordingly.

What is the link to the Apache JIRA

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

How was this patch tested?

Verified using TestOMDirectoryCreateResponseV1.java and TestOzoneDirectory.java unit test cases.
Note: As this is an incremental change, it is expected to have failures related to file, key ops test cases. Will fix this later through subsequent sub-tasks.

[linyiqun]

With new layout version added, the latest PR logic looks more clear and independent.
I just left my minor comments. I am thinking one next part of work is delete dir implementation, right? Only seeing create dir implementation here.
In additional, please have a look of the github build failure, it doesn't show green.

[linyiqun]

Wrong log instance name, TestOzoneFileSystem -> TestOzoneDirectory

[rakeshadr]

Will do it. Thanks!

[linyiqun]

Just a comment: will we introduced DIR_LOCK for dir table update in the future?

[rakeshadr]

Yes, exactly. This is long term goal and will kick start once the primary changes are IN.

[rakeshadr]

With new layout version added, the latest PR logic looks more clear and independent.
I just left my minor comments. I am thinking one next part of work is delete dir implementation, right? Only seeing create dir implementation here.
In additional, please have a look of the github build failure, it doesn't show green.

Will fix TestOzoneConfigurationFields test case.
Next task am planning to do CreateFile and then get/listStatus. IMHO, both file/dir delete can be done together in one patch which involves changes in KeyDeletingService.

Does this make sense to you.

Thanks for the reviews!

[linyiqun]

Next task am planning to do CreateFile and then get/listStatus. IMHO, both file/dir delete can be done together in one patch which involves changes in KeyDeletingService.

Does this make sense to you.

Makes sense to me.

[linyiqun]

+1 from me.

[bharatviswa504]

I see we used layOutVersion and tested the change. But this might cause failures to other test like when key and directory with same name, as we have not used new logic in KeyCreate request. (Some of these tests are added in master branch with HDDS-4155.
Let's add individual classes with UT's and once basic classes like Key Creation/and others are done then we can integrate it to OM.

[bharatviswa504]

Thank You @rakeshadr for the contribution.
I have few comments

[rakeshadr]

I see we used layOutVersion and tested the change. But this might cause failures to other test like when key and directory with same name, as we have not used new logic in KeyCreate request. (Some of these tests are added in master branch with HDDS-4155.
Let's add individual classes with UT's and once basic classes like Key Creation/and others are done then we can integrate it to OM.

Sure, how about adding this UTs along with File/Key creation patch?. As we know, here its only the dir changes.

[bharatviswa504]

I see we used layOutVersion and tested the change. But this might cause failures to other test like when key and directory with same name, as we have not used new logic in KeyCreate request. (Some of these tests are added in master branch with HDDS-4155.
Let's add individual classes with UT's and once basic classes like Key Creation/and others are done then we can integrate it to OM.

Sure, how about adding this UTs along with File/Key creation patch?. As we know, here its only the dir changes.

As any way we are testing at UT, we can add the entries to KeyTable and implement/simulate these tests. (Similar to Old Requests)

[rakeshadr]

Thanks a lot @bharatviswa504 for the useful review comments and your time. I will upstream next commit.

[rakeshadr]

As any way we are testing at UT, we can add the entries to KeyTable and implement/simulate these tests. (Similar to Old Requests)

Sure, will do that way.

[rakeshadr]

I've updated another patch addressing @bharatviswa504's comments.

[bharatviswa504]

Thank You for the update @rakeshadr
I have a few more comments.

[rakeshadr]

Thanks again @bharatviswa504 for the comments. Uploaded patch addressing the comments. Please let me know the feedback!

[bharatviswa504]

Thank You @rakeshadr for the update.
Overall LGTM. I have a few comments/questions.

I have resolved old comments, as it will help no to miss the comments to address them.

[bharatviswa504]

We are adding them 2 times once here, and next immediately in addDirectoryTableCacheEntries.
No correctness issue, but can be removed from one place.

[rakeshadr]

Sure, I will remove it in my new class.

FYI, looks like existing code OMDirectoryCreateRequest.java#L276 has this issue. Like you said, there is no harm anyway and am not touching the current code.

[bharatviswa504]

Yes, looks like we can remove from existing code.
Let's open a new jira to fix this.

[bharatviswa504]

For leaf, we have used baseObjID + objCount. But here it is mentioned we use baseObjID is used for leaf, assuming that it has started for missing directories from baseObjID +1.

pathInfo.setLeafNodeObjectId(baseObjId + objectCount);

If we want to keep them sorted, we can start with ObjCount as 0. (Not sure, in future if this will help in the listing)

With this, there is a chance to exceed maxObjectID

[rakeshadr]

Good point. I've used existing logic from code OMDirectoryCreateRequest#254.

I will add two more UTs to verify it. Also, I will remove the wrong comment. Hope this is fine?

1. testCreateDirectoryExceedLimitOfMaxLevels255
2. testCreateDirectoryUptoLimitOfMaxLevels255

Say,    baseObjId = 25600
        maxObjId  = 25855
        maxLevels = 25855 - 25600 = 255
Since the exit condition is "if (nextObjId > maxObjId)", it will exit and throw exception once
the value of nextObjId becomes 25856, which will be used for the leaf node.

[bharatviswa504]

+1 LGTM.
Thank You @rakeshadr for multiple updates and @linyiqun for the review.

[rakeshadr]

Thank you @bharatviswa504 and @linyiqun for help in reviews and provided really useful comments.