HDDS-4683. ListKeys: do lookup in dir and file tables by rakeshadr · Pull Request #1811 · apache/ozone

rakeshadr · 2021-01-17T10:45:49Z

What changes were proposed in this pull request?

This task is to perform #listKeys by searching the startKey, keyPrefix in Dir & File Tables.

What is the link to the Apache JIRA

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

How was this patch tested?

Added UT.

…e#1404)

…le entry into separate FileTable (apache#1473)

…pache#1503)

…try in dir/file table (apache#1557) HDDS-4357: Rename : make rename an atomic ops by updating key path entry in dir/file table

…nges into the branch (apache#1668)

…rom the OM DB Definition. (apache#1724)

…given path (apache#1679)

…ries in the key path (apache#1764)

linyiqun

@rakeshadr , haven't started the review. As I find there is one another similar task HDDS-4332(ListFileStatus - do lookup in directory and file tables). What's the major differences in these two task implementation? That would be helpful to review this PR if I know this.

rakeshadr · 2021-01-18T09:28:37Z

@rakeshadr , haven't started the review. As I find there is one another similar task HDDS-4332(ListFileStatus - do lookup in directory and file tables). What's the major differences in these two task implementation? That would be helpful to review this PR if I know this.

Thank you @linyiqun for your help in reviews. Following are the points I learned after comparing listKeys and listStatus:

ListKeys will skip the given startKey and won't be added to the finalList
ListKeys will add trailing slash for the directory key names.
ListKeys will recursively list all the keys starting from keyPrefix and startKey.

For ex: FS structure like, 

/a
/a/b
/a/b/c
/a/b/d

/a/b/file1
a/b/c/file2

#listKeys("/a/b") returns array values like, [a/b/, a/b/file1, a/b/c/, a/b/d/, a/b/c/file2]

linyiqun

Thanks @rakeshadr for the comments.
I take the initial review. Some places still not fully get the point. Please have a look.

linyiqun · 2021-01-18T12:34:25Z

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

-      startKey = OmUtils.normalizeKey(startKey, true);
-      keyPrefix = OmUtils.normalizeKey(keyPrefix, true);
+      startKey = OmUtils.normalizeKey(startKey, preserveTrailingSlash);
+      keyPrefix = OmUtils.normalizeKey(keyPrefix, preserveTrailingSlash);


Not fully get this, why we don't preserveTrailingSlash once isOmLayoutVersionV1 is enabled.

Thanks @linyiqun for the comments.

Yes, this logic is not required. I will remove this unnecessary isOmLayoutVersionV1 check in the PR.

I was having a thinking that, in V1 there won't be any trailing slash in the "keyName". As we know, in existing code a trailing slash in the keyName represents a directory path in KeyTable.

linyiqun · 2021-01-18T12:35:51Z

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

+        }
+        if (!StringUtils.equals(dirName, keyPrefix)) {
+          listKeysRecursivelyFromKeyPrefixV1(volumeName, bucketName, dirName,
+                  maxKeys, keyList);


maxKeys should be updated when doing recursively list keys

maxKeys represents the limit and the batch size. During iteration and seeks, keyList.size() shouldn't exceed this limit. So, maxKeys is static.

keyList.size() <= maxKeys

Okay, one improvement I am thinking: can we do a check if keyList.size() already reach maxKeys, if reached, no need to call listKeysRecursivelyFromKeyPrefixV1 for dir and can break the loop. This can avoid unnecessary listKeysRecursivelyFromKeyPrefixV1 be invoked for subsequent sub-dirs.

Agreed, will do the check.

linyiqun · 2021-01-18T13:04:42Z

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

+      // Iteration-2:
+      // startKeyPos="dir1/dir2/dir3", parentKeyPrefix="dir1/dir2"
+      // Iteration-3:
+      // startKeyPos="dir1/dir2", parentKeyPrefix="dir1"


Re-Edit my previous comments:
In the recursively search of startKey dir1/dir2/dir3, dir1/dir2, will it add the repeated entries in parent search?

E.g.
There are 5 files:
dir1/dir2/dir3/file1
dir1/dir2/dir3/file2
dir1/dir2/dir3/file3
dir1/dir2/dir3/file4

Iteration-1:
startKeyPos="dir1/dir2/dir3/file3", parentKeyPrefix="dir1/dir2/dir3"
keylist[dir1/dir2/dir3/file4]

Iteration-2:
startKeyPos="dir1/dir2/dir3", parentKeyPrefix="dir1/dir2"
keylist[dir1/dir2/dir3/file4, dir1/dir2/dir3/file1, dir1/dir2/dir3/file2, dir1/dir2/dir3/file3, dir1/dir2/dir3/file4]

In Iteration-2, will it search for the dir1/dir2/dir3 again? I think it will be skipped as dir1/dir2/dir3 is the new startKey will be skipped.

Thanks @linyiqun for the example case. Please see my below reply.

In Iteration-2, will it search for the dir1/dir2/dir3 again? I think it will be skipped as dir1/dir2/dir3 is the new startKey will be skipped.

Yes, it will skip the startKey from the resultList when startKey is not empty. Also, it won't search all the files under dir1/dir2/dir3. Please read the below paragraph for the algorithm used in this patch.

Reason: the listKeysV1() algorithm is written like a template fashion with the following steps irrespective of startKey empty or non-empty:
step-1) It will fetch all the files under the given path.
step-2) It will fetch all the directories under the given path.

If the startKey is a directory, then the assumption is that all the files under this key was already visited in previous batch. So, in your example, you have passed startKeyPos="dir1/dir2/dir3". Here it will check any nextString directory exists after the startKeyPos in DirTable. In your example case, we don't have any , so will return empty dir list. Assume, there was "dir1/dir2/dir4" exists, then it would have added to the result list.
Now, it won't go to FileTable and list all keys after startKeyPos="dir1/dir2/dir3", instead it will move startKeyPos pointer to previous dir "dir1/dir2" and do a check !StringUtils.equals(parentStartKeyPath, keyPrefix). This return false and we have finished the listing key operation.

So the resultList in Iteration-2 case is, empty list []

Hope this explanation is clear to you.

linyiqun · 2021-01-18T13:06:44Z

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

+  protected List<OzoneFileStatus> listOzoneFileStatusesV1(boolean recursive,
+      String startKey, int countEntries, long numEntries, String clientAddress,
+      String volumeName, String bucketName, String keyName,
+      boolean sortAddress, boolean listKeys) throws IOException {


Can we update this as a private method, I see this is privately used in this class.

linyiqun · 2021-01-19T15:56:12Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestObjectStoreV1.java

  }

+  @Test
+  public void testListKeysWithNotNormalizedPath() throws Exception {


Current test case only cover few cases of listKeysV1. Actually like startKey, maxKeys of listKeys are not tested. However, I also didn't find a good unit test added for original listKeys call.

Yes, I will add more test cases. I'm thinking something like large dirs and sub-dirs/files.

Also, there is an open point: #listKeys("key-a-"), basically partial keyName and startKey. For example, actual keyName stored in DB are "key-a-10", "key-a-11", "key-a-12"..."key-z-10", "key-z-11", "key-z-12".
I will raise separate jira and work it on.

linyiqun

@rakeshadr , I read the doc of listKey algorithm. And some comments I left below. Please have a look.

linyiqun · 2021-01-25T03:38:06Z

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

+      }
+    } else {
+      // TODO: Handle startKey not starting with keyPrefix. Should we suport
+      //  this case?


If startKey does not start with key prefix, we can just compare the their value.

If key prefix already behind the startKey, this search equals to the listKeys(key prefix , startKey='') as all entries with key prefix should behind the startKey.

If key prefix not behind the startKey, the empty list expected to be returned since startKey behind all the prefix key entries.

Does this handling make the sense here?

mukul1987 · 2021-02-07T17:50:51Z

@rakeshadr I think the patch needs to be rebased, there are multiple unrelated changes.

rakeshadr · 2021-02-23T12:43:49Z

Thanks a lot @linyiqun for the detailed reviewing on this patch.

On a second thought, I'd like to avoid more complex logic on the server side. I thought of re-using the existing proxy#listStatus() client API and making recursion logic at the client side. I've created a new PR-1954. I'd greatly appreciate your feedback on that. Thanks again!

Its just a first implementation and I think few more follow up jira tasks required to fully refine the client side logic by introducing batch fetching on levels incrementally etc etc. Will plan that work one-by-one.

linyiqun · 2021-02-23T15:15:27Z

On a second thought, I'd like to avoid more complex logic on the server side. I thought of re-using the existing proxy#listStatus() client API and making recursion logic at the client side. I've created a new PR-1954. I'd greatly appreciate your feedback on that. Thanks again!

@rakeshadr . Will have a look recent days.

rakeshadr and others added 11 commits January 13, 2021 09:30

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

eaf6859

…e#1404)

HDDS-4266: CreateFile : store parent dir entries into DirTable and fi…

9aca1d5

…le entry into separate FileTable (apache#1473)

HDDS-4332: ListFileStatus - do lookup in directory and file tables (a…

a7d568f

…pache#1503)

HDDS-4357: Rename : make rename an atomic ops by updating key path en…

b416406

…try in dir/file table (apache#1557) HDDS-4357: Rename : make rename an atomic ops by updating key path entry in dir/file table

HDDS-4358: Delete : make delete an atomic operation (apache#1607)

eb84c94

HDDS-4321. Fix compilation errors : merge HDDS-4308 and HDDS-4473 cha…

0be5c10

…nges into the branch (apache#1668)

HDDS-4596. Directory table, fileTable and openFile Table is missing f…

31cb08f

…rom the OM DB Definition. (apache#1724)

HDDS-4514. AllocateBlock : lookup and update open file table for the …

62d01b8

…given path (apache#1679)

HDDS-2942. Putkey : create key table entries for intermediate directo…

af7d54c

…ries in the key path (apache#1764)

HDDS-4658. LookupKey: do lookup in dir and file tables (apache#1775)

3825571

HDDS-4683. ListKeys: do lookup in dir and file tables

479a4af

linyiqun reviewed Jan 17, 2021

View reviewed changes

linyiqun reviewed Jan 18, 2021

View reviewed changes

linyiqun reviewed Jan 19, 2021

View reviewed changes

linyiqun reviewed Jan 25, 2021

View reviewed changes

rakeshadr force-pushed the HDDS-2939 branch 3 times, most recently from ea6116a to 37178c5 Compare February 1, 2021 05:37

rakeshadr force-pushed the HDDS-2939 branch 3 times, most recently from bc6d651 to ec62cdb Compare February 5, 2021 09:21

rakeshadr force-pushed the HDDS-2939 branch 2 times, most recently from 2c7fc4a to e01a1ef Compare February 15, 2021 05:55

rakeshadr force-pushed the HDDS-2939 branch from 1a78039 to 1de4738 Compare February 18, 2021 03:13

rakeshadr closed this Feb 23, 2021

Comments

Conversation

rakeshadr commented Jan 17, 2021

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

linyiqun 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!

rakeshadr commented Jan 18, 2021

Uh oh!

linyiqun 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 Jan 19, 2021 • 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 Jan 18, 2021 • 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!

rakeshadr Jan 27, 2021 • 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!

linyiqun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mukul1987 commented Feb 7, 2021

Uh oh!

rakeshadr commented Feb 23, 2021

Uh oh!

linyiqun commented Feb 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

linyiqun left a comment •

edited

Loading

linyiqun Jan 19, 2021 •

edited

Loading

linyiqun Jan 18, 2021 •

edited

Loading

rakeshadr Jan 27, 2021 •

edited

Loading