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

rakeshadr · 2020-07-21T12:27:50Z

What changes were proposed in this pull request?

All the directories from the path prefix should be created on 'prefixTable'

What is the link to the Apache JIRA

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

How was this patch tested?

Verified using TestOMDirectoryCreateResponse.java and TestOzonePrefixTable.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.

bharatviswa504 · 2020-07-31T18:11:19Z

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmPrefixInfo.java

+  private long modificationTime;
  private List<OzoneAcl> acls;

  public OmPrefixInfo(String name, List<OzoneAcl> acls,


This constructor can be removed, not being used anymore.

Agreed, will fix it new commit

bharatviswa504 · 2020-07-31T18:25:02Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java

+  }
+
+  @Override
+  public String getOzonePrefixKey(long parentObjectId, String prefixName) {


Both logic seems to be same. Do we need 2 methods? If needed, one method can call other to avoid duplicate code.

Agreed, will fix it in new commit

bharatviswa504 · 2020-07-31T18:44:19Z

...ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java

+      // 3. Add 'sub-dir' to missing parents list
+      String dbNodeName = omMetadataManager.getOzonePrefixKey(lastKnownParentId,
+              fileName);
+      OmPrefixInfo omPrefixInfo = omMetadataManager.getPrefixTable().


This gets value from DB, as the prefix table does not use table cache, it stores entries in prefixTree.
I think here we need to expose an API in prefixManagerImpl to get(String Path) and use that here.
And also a question, do we need hold prefix lock here?

And also got a question here.

When prefixAcl is removed, we remove paths. That can cause the problem here right, the paths which are created by mkdir now can be removed by removePrefixAcl.

Very good point. Based on the upgrade path discussion, the plan is to introduce new directory table. In that case, all the dir ops will be under BUKCET_LOCK. Will update the patch soon with that changes.

elek · 2020-08-24T15:49:13Z

/pending review comments are not addressed

github-actions

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

review comments are not addressed

linyiqun · 2020-08-31T03:35:27Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java


+  /**
+   * Given a volume, bucket and a key, return the corresponding DB key.
+   *


The Comment needs to be updated, here we don't pass volume, bucket and key. The same problem in getOzoneLeafNodeKey method.

Will fix it in new commit.

linyiqun · 2020-08-31T03:40:18Z

...-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java

      // cannot create a directory with the given key.
+      // Verify the path against prefix table
      OMFileRequest.OMPathInfo omPathInfo =
-          OMFileRequest.verifyFilesInPath(omMetadataManager, volumeName,


As we store the dir in prefix table now, we need to acquire PREFIX_LOCK instead of BUCKET_LOCK.
acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName);

Very good point. Will improve this logic.

linyiqun · 2020-08-31T06:08:43Z

...op-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzonePrefixTable.java

+            Table.KeyValue<String, OmKeyInfo>> keyTableItr =
+            omMgr.getKeyTable().iterator();
+    while (keyTableItr.hasNext()) {
+      fail("Shouldn't add any entries in KeyTable!");


Can you add an additionally test cases for creating one file and verified current entries in KeyTable? Now the new file key would be like objId/fileName rather than /volume/bucket/keyName.

#verifyPrefixKey() function is doing the same by fetching entries using "omMgr.getPrefixTable().get(dbKey);" call.
Are you suggesting something different from this verification check?

@rakeshadr , actually I mean we need to add logic to verify new entry on key table as well in ut. Current unit test only check the dir entry in prefix table.

My suggestion is to write a new key file here.
Then we can check the new format entry in key table.

Key table format before:
KeyName:
/vol1/buck1/a/b/c/d/file1

Now will be like this:
1028/file1

Thanks for the details. I have created new PR #1404 and incorporated your comments. Please review it.

rakeshadr

Thanks a lot @bharatviswa504, @linyiqun for the reviews

rakeshadr · 2020-09-02T09:06:34Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java


+  /**
+   * Given a volume, bucket and a key, return the corresponding DB key.
+   *


Will fix it in new commit.

rakeshadr · 2020-09-02T09:07:28Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java

+  }
+
+  @Override
+  public String getOzonePrefixKey(long parentObjectId, String prefixName) {


Agreed, will fix it in new commit

rakeshadr · 2020-09-02T09:14:24Z

...op-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzonePrefixTable.java

+            Table.KeyValue<String, OmKeyInfo>> keyTableItr =
+            omMgr.getKeyTable().iterator();
+    while (keyTableItr.hasNext()) {
+      fail("Shouldn't add any entries in KeyTable!");


#verifyPrefixKey() function is doing the same by fetching entries using "omMgr.getPrefixTable().get(dbKey);" call.
Are you suggesting something different from this verification check?

rakeshadr · 2020-09-02T09:15:31Z

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmPrefixInfo.java

+  private long modificationTime;
  private List<OzoneAcl> acls;

  public OmPrefixInfo(String name, List<OzoneAcl> acls,


Agreed, will fix it new commit

rakeshadr · 2020-09-02T09:24:55Z

...-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java

      // cannot create a directory with the given key.
+      // Verify the path against prefix table
      OMFileRequest.OMPathInfo omPathInfo =
-          OMFileRequest.verifyFilesInPath(omMetadataManager, volumeName,


Very good point. Will improve this logic.

rakeshadr · 2020-09-02T09:27:40Z

...ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java

+      // 3. Add 'sub-dir' to missing parents list
+      String dbNodeName = omMetadataManager.getOzonePrefixKey(lastKnownParentId,
+              fileName);
+      OmPrefixInfo omPrefixInfo = omMetadataManager.getPrefixTable().


Very good point. Based on the upgrade path discussion, the plan is to introduce new directory table. In that case, all the dir ops will be under BUKCET_LOCK. Will update the patch soon with that changes.

rakeshadr · 2020-09-07T17:20:18Z

Thanks @bharatviswa504 @linyiqun for the reviews. I am sorry for multiple PRs. I have created new PR #1404 now. Basically, the idea is to create new set of classes V1 for easy migration of this feature branch with rolling upgrade. Also, I have created new table Directory_Table instead of Prefix Table. This is to simplifies the caching logic later.

Please review this PR and thanks for your patience!

mukul1987 · 2020-09-21T18:41:28Z

@rakeshadr Can we close this PR ? as there is a new one available ?

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

561332e

xiaoyuyao self-requested a review July 24, 2020 03:52

Merge branch 'HDDS-2939' into HDDS-2949-new

af90e52

bharatviswa504 reviewed Aug 3, 2020

View reviewed changes

github-actions bot requested changes Aug 24, 2020

View reviewed changes

github-actions bot added the pending label Aug 24, 2020

linyiqun reviewed Aug 31, 2020

View reviewed changes

rakeshadr commented Sep 2, 2020

View reviewed changes

mukul1987 closed this Sep 21, 2020

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

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

Uh oh!

Conversation

rakeshadr commented Jul 21, 2020

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

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!

elek commented Aug 24, 2020

Uh oh!

github-actions bot left a comment

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!

linyiqun Aug 31, 2020 • 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!

linyiqun Sep 2, 2020 • 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!

rakeshadr left a comment

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!

rakeshadr commented Sep 7, 2020

Uh oh!

mukul1987 commented Sep 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

linyiqun Aug 31, 2020 •

edited

Loading

linyiqun Sep 2, 2020 •

edited

Loading