-
Notifications
You must be signed in to change notification settings - Fork 499
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-11205. Implement a search feature for users to locate keys pending Deletion within the OM Deleted Keys Insights section #6969
Conversation
…ng Deletion within the OM Deleted Keys Insights section.
@devmadhuu @dombizita Please take a look! |
For FSO buckets, how does the search work if the parent directories or even the volume and bucket have been deleted already? The key may still be pending deletion but the prefix will have no matches once the parents are cleared. |
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 working on this @ArafatKhan2198, overall it looks good to me, I had one idea, let me know what you think of it. Also could you answer to @errose28's comment?
keyIter.seek(startPrefix); | ||
@GET | ||
@Path("/deletePending/search") | ||
public Response searchDeletedKeys( |
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.
The beginning and ending of this method have a lot in common with searchOpenKeys
.
Lines 110 to 117 in 350a340
@GET | |
@Path("/open/search") | |
public Response searchOpenKeys( | |
@DefaultValue(DEFAULT_START_PREFIX) @QueryParam("startPrefix") | |
String startPrefix, | |
@DefaultValue(RECON_OPEN_KEY_DEFAULT_SEARCH_LIMIT) @QueryParam("limit") | |
int limit, | |
@DefaultValue(RECON_OPEN_KEY_SEARCH_DEFAULT_PREV_KEY) @QueryParam("prevKey") String prevKey) throws IOException { |
Can we extract them? Do you think it would make sense?
Thanks for the comment @errose28 The DeletedTable has a structure similar to that of the key table, following this pattern:
When a key is deleted and placed inside the
The search operation leverages the Even if the bucket or directory is deleted, it does not impact the search because we are only seeking values in the Additionally, I have verified how each record key is stored in the For example, if the user provides a search prefix like volume/bucket1/dir1/, the following record shown in the image, along with any other record that matches this prefix, will be collected and returned. |
@dombizita @devmadhuu @errose28 |
Pls fix the checkstyle issue. |
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 @ArafatKhan2198 for improving the patch. Changes LGTM +1. Just fix your checkstyle issue.
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 @ArafatKhan2198 for the changes, looks good to me.
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Outdated
Show resolved
Hide resolved
…improved pagination and prefix handling.
Thanks @devabhishekpal @devmadhuu @sumitagrawl for the review and comments on this PR please consider the latest changes :- |
Thanks for the review @devmadhuu @sumitagrawl @devabhishekpal please consider the latest commit having the final changes as discussed offline In the latest commit, we made significant improvements to the search functionality for deleted keys:
|
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightEndpoint.java
Outdated
Show resolved
Hide resolved
… keys from their respective tables
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.
LGTM
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 @ArafatKhan2198 for improving the patch. Few comments left.
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightEndpoint.java
Outdated
Show resolved
Hide resolved
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Outdated
Show resolved
Hide resolved
Please take another look @devmadhuu |
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconUtils.java
Outdated
Show resolved
Hide resolved
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 @ArafatKhan2198 for improving the patch. Just few nits and one old comment not yet addressed.
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightEndpoint.java
Outdated
Show resolved
Hide resolved
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Outdated
Show resolved
Hide resolved
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.
One minor comment.
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Show resolved
Hide resolved
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.
Pls check , some inconsistency for blank value check of startPrefix
at some places but not a some places.
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightEndpoint.java
Show resolved
Hide resolved
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.
Pls check my comment.
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightEndpoint.java
Show resolved
Hide resolved
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.
Some minor comment.
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconUtils.java
Show resolved
Hide resolved
@devmadhuu Without this null check, validateStartPrefix would return false because it mistakenly interprets the empty startPrefix as a volume-level search, which isn't allowed. By adding a check for an empty string, we explicitly allow this condition where startPrefix is
This adjustment ensures the search behaves correctly when startPrefix is empty, avoiding unnecessary errors. |
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 @ArafatKhan2198 for improving the patch. Changes LGTM +1. Kindly check that API 404 error when we landup on the keyPendingForDeletion page in Recon UI. And I understand that for openKeys search API, you will be fixing things in follow up PR. Rest all looks good.
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.
Changes LGTM now. Thanks @ArafatKhan2198
What changes were proposed in this pull request?
This pull request enhances the search functionality for deleted pending keys in the Deleted Keys Insights section. The previous implementation was limited to a subset of the available data returned by the API. This update ensures accurate retrieval of specific key information by improving pagination and handling of both prefix-based searches and general search scenarios.
The patch implements the ability to search for deleted keys using a specified prefix, efficiently returning results that match the prefix across both FSO and Object Store layouts. The logic has been refactored to the OMDBInsightEndpoint, unifying the method for handling all cases where the startPrefix is provided or not.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11205
How was this patch tested?
1inkjkisd
which is underfso-bucket
.