-
Notifications
You must be signed in to change notification settings - Fork 509
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-10634. Recon - listKeys API for listing of OBS , FSO and Legacy bucket keys with filters. #6503
Conversation
…bucket keys with filters.
I think default replication types should be all of them. This is consistent with the key size and create time filters which have no effect if no value is provided.
|
|
I think just providing the last key in the list as a start key might be easier to resume pagination from than adding a count based offset. |
@dombizita @ArafatKhan2198 @sodonnel @sumitagrawl Pls review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort on this @devmadhuu.
Left a few comments.
@@ -134,6 +134,7 @@ public class TestNSSummaryEndpointWithLegacy { | |||
private static final String BUCKET_TWO = "bucket2"; | |||
private static final String BUCKET_THREE = "bucket3"; | |||
private static final String BUCKET_FOUR = "bucket4"; | |||
private static final String BUCKET_FIVE = "bucket5"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we asserting anything in the test class TestNSSummaryEndpointWithLegacy
? We have created a bunch of new files and directories but have not asserted anything yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we asserting anything in the test class
TestNSSummaryEndpointWithLegacy
? We have created a bunch of new files and directories but have not asserted anything yet.
I added the test case for both legacy and obs buckets in TestNSSummaryEndpointWithOBSAndLegacy
class. Will remove the tree structure from here.
// There are no sub-paths under this LEGACY bucket. | ||
assertEquals(2, duBucketResponse.getCount()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are mentioning that there are no sub paths under this legacy bucket but we are asserting that the subPathCount
returned by duBucketResponse.getCount()
returns 2
Or does the comment is mentioning something else that I have mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is not correct. Will update it.
@@ -145,6 +146,12 @@ public class TestNSSummaryEndpointWithLegacy { | |||
private static final String KEY_NINE = "dir5/file9"; | |||
private static final String KEY_TEN = "dir5/file10"; | |||
private static final String KEY_ELEVEN = "file11"; | |||
private static final String KEY_TWELVE = "file12"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also update the tree diagram for TestNSSummaryEndpointWithLegacy
with the new changes. I think we could implement a similar diagram style like present in TestNSSummaryEndpointWithOBSAndLegacy
to show case the file hierarchy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also update the tree diagram for
TestNSSummaryEndpointWithLegacy
with the new changes. I think we could implement a similar diagram style like present inTestNSSummaryEndpointWithOBSAndLegacy
to show case the file hierarchy.
I added the test case for both legacy and obs buckets in TestNSSummaryEndpointWithOBSAndLegacy
class. Will remove the tree structure from here.
* @return The constructed full path of the key as a String. | ||
* @throws IOException | ||
*/ | ||
public static String constructFullPath(OmKeyInfo omKeyInfo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, needs to rebase it.
Since this endpoint |
Ok, sure will add integration test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devmadhuu Thanks for working over this, IMO, we need use different approach for listing files
|
||
Stats stats = new Stats(limit); | ||
|
||
duResponse = handler.getListKeysResponse(stats, recursive); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of overloading DuReponse class, we can define new response class for listFile purpose and return only required information
if (stats.getCurrentCount() < stats.getLimit()) { | ||
populateDiskUsage(keyInfo, diskUsageList); | ||
stats.setCurrentCount(stats.getCurrentCount() + 1); | ||
stats.setLastKey(kv.getKey()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for list file, LastKey can be one of file, but seems current logic do not support iteration continue from that point, eg:
- lets list file for a volume /a
- limit reached for one of file /a/b/c/d/1.txt and this is last key
- How this will continue listing further keys and other bucket files if given as input ?
Shall we use other listing operation as we use from rocks db iterator ? which is lexicographic ordering ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sumitagrawl , thanks for reviewing the patch. Current listKeys API has this limit parameter which will limit in order. If other buckets also present in volume and if limit reached, i will not list. This is similar to what we have in CLI behavior.
What changes were proposed in this pull request?
This PR adds a new API in Recon for listing keys for OBS buckets, Legacy buckets with filters and recursively in a flat structure for FSO buckets.
New API:
api/v1/namespace/listKeys?startPrefix=/volume1/obs-bucket/&count=105
Default values of API parameters if not provided:
Behavior of API:
For OBS bucket - list out count number of keys on the provided path.
This API will implement pagination support using count params.
Get List of All Keys:
GET /api/v1/namespace/listKeys
Input Request for LEGACY bucket:
Output Response:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10634
How was this patch tested?
Added Junit test cases and tested various assertions.