Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HDDS-4332: ListFileStatus - do lookup in directory and file tables #1503

Merged
merged 7 commits into from Oct 31, 2020

Conversation

rakeshadr
Copy link
Contributor

What changes were proposed in this pull request?

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

What is the link to the Apache JIRA

This task is to perform look up of the user given key path in the directory, file and openFile tables.
OzoneFileSystem APIs:

  • GetFileStatus
  • listStatus

How was this patch tested?

Added UTs - TestHadoopDirTreeGeneratorV1, TestOzoneFileInterfacesV1, TestOzoneFileSystemV1...

Copy link
Contributor

@linyiqun linyiqun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rakeshadr , thanks for working on this. Initial review comments from me.
Please have a look for this when you get a chance.

@rakeshadr
Copy link
Contributor Author

@rakeshadr , thanks for working on this. Initial review comments from me.
Please have a look for this when you get a chance.

Thanks a lot for the review comments. I will update the PR.

Copy link
Contributor

@linyiqun linyiqun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the PR, @rakeshadr . One further review comment below.

In additional, current test change not fully cover the test for listStatusV1. Example, current OzoneFS test change doesn't address the case for listStatus with other startKey specified.
I see the test class TestKeyManagerImpl does the good coverage for listStatus call, can we add a test unit like that or make a minor refactor based on that?

@rakeshadr
Copy link
Contributor Author

Thanks for updating the PR, @rakeshadr . One further review comment below.

In additional, current test change not fully cover the test for listStatusV1. Example, current OzoneFS test change doesn't address the case for listStatus with other startKey specified.
I see the test class TestKeyManagerImpl does the good coverage for listStatus call, can we add a test unit like that or make a minor refactor based on that?

Thanks @linyiqun . Thats really a good point. I also looked at keyManagerImpl UT and it requires some refactoring effort to modify the createDir and other logic. I will work on and update you.

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank You @rakeshadr for the contribution.
I have reviewed code changes, have few comments, pending test changes review.

@rakeshadr
Copy link
Contributor Author

Thank You @rakeshadr for the contribution.
I have reviewed code changes, have few comments, pending test changes review.

Thanks a lot @bharatviswa504 for the review comments. I've updated the PR. Appreciate if you could provide clarifications on few of the comments.

@rakeshadr
Copy link
Contributor Author

Thanks for updating the PR, @rakeshadr . One further review comment below.

In additional, current test change not fully cover the test for listStatusV1. Example, current OzoneFS test change doesn't address the case for listStatus with other startKey specified.
I see the test class TestKeyManagerImpl does the good coverage for listStatus call, can we add a test unit like that or make a minor refactor based on that?

I've created HDDS-4412 jira to refactor TestKeyManagerImpl unit testing. Today, it uses stale APIs and IMHO will refactor this in a separate task to make this patch comparatively smaller:-)

@rakeshadr
Copy link
Contributor Author

Thanks @linyiqun for the detailed reviews and your quick responses really helped a lot. Appreciate if you could provide your overall vote/confirmation about the patch.

@linyiqun
Copy link
Contributor

I've created HDDS-4412 jira to refactor TestKeyManagerImpl unit testing. Today, it uses stale APIs and IMHO will refactor this in a separate task to make this patch comparatively smaller:-)

Let's track that on HDDS-4412,

Thanks @linyiqun for the detailed reviews and your quick responses really helped a lot. Appreciate if you could provide your overall vote/confirmation about the patch.

+1. Give my approval, : ).

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HDDS-4391 using Paths object has performance disadvantage, so can we see we can use Hadoop FS Path, as Paths use FileSystem and then initialize UnixPath.

We can look into this in further Jira.

One minor comment, take care of that in test opened Jira for refactoring.

@bharatviswa504 bharatviswa504 merged commit d74b856 into apache:HDDS-2939 Oct 31, 2020
@bharatviswa504
Copy link
Contributor

Thank You @rakeshadr for the contribution and @linyiqun for the review.

@rakeshadr
Copy link
Contributor Author

HDDS-4391 using Paths object has performance disadvantage, so can we see we can use Hadoop FS Path, as Paths use FileSystem and then initialize UnixPath.

We can look into this in further Jira.

One minor comment, take care of that in test opened Jira for refactoring.

Noted these points and will take care in the following jira tasks.

Thanks a lot @bharatviswa504, @linyiqun for the continuous support in reviews and committing the changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants