-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-10389. Implement a search feature for users to locate open keys within the Open Keys Insights section. #6231
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
Conversation
Implement a search feature for users to locate open keys within the Open Keys Insights section.
devmadhuu
left a comment
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 working on this patch. Have few comments. Pls check.
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
devmadhuu
left a comment
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.
Have some more review comments. Pls check.
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Show resolved
Hide resolved
...one/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestOMDBInsightSearchEndpoint.java
Outdated
Show resolved
Hide resolved
devmadhuu
left a comment
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 , if review comments are take care. I can see some review comments are still open.
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Outdated
Show resolved
Hide resolved
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Show resolved
Hide resolved
devmadhuu
left a comment
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 continue working on this patch, however some old comments still open and some new comments. Pls check.
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconConstants.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconConstants.java
Outdated
Show resolved
Hide resolved
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Show resolved
Hide resolved
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Show resolved
Hide resolved
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Outdated
Show resolved
Hide resolved
...one/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestOMDBInsightSearchEndpoint.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
|
@devmadhuu @dombizita |
dombizita
left a comment
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, please take a look at the minor comments inline!
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Outdated
Show resolved
Hide resolved
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Show resolved
Hide resolved
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Outdated
Show resolved
Hide resolved
devmadhuu
left a comment
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 , one more comment.
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Outdated
Show resolved
Hide resolved
devmadhuu
left a comment
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.
@ArafatKhan2198 thanks for taking care pagination scenario. few comments. Pls check.
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.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
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Outdated
Show resolved
Hide resolved
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Show resolved
Hide resolved
...one/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestOMDBInsightSearchEndpoint.java
Show resolved
Hide resolved
...one/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestOMDBInsightSearchEndpoint.java
Show resolved
Hide resolved
|
Thanks for the initial round of reviews, @devmadhuu and @dombizita. Could you please check the patch again? I have added more tests to cover every possible scenario that the search functionality might encounter. Here is a short summary of various paths and scenarios tested in the test class:
|
|
@devmadhuu @dombizita Please take a look |
Explanation of the
|
dombizita
left a comment
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, especially on the detailed comments on the PR and in the code, it made the review much easier. I found one comment that you may not addressed, otherwise I'm fine with the changes.
...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/OMDBInsightSearchEndpoint.java
Show resolved
Hide resolved
The IllegalArgumentException in the given code can occur due to invalid arguments passed to methods. In the context of the convertToObjectPath method and the search logic, possible scenarios include: Invalid Volume or Bucket Names: The validateNames method can throw an IllegalArgumentException if volume or bucket names do not meet required criteria. When converting path components to IDs, if the format is incorrect or if the conversion logic encounters unexpected values, it might throw an IllegalArgumentException. I'll mention it in the Javadoc as well. |
devmadhuu
left a comment
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 a nit. Pls check. Else LGTM +1
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconUtils.java
Outdated
Show resolved
Hide resolved
|
Thanks for working on it @ArafatKhan2198! Thanks for the review @devmadhuu! |
…-delete * HDDS-10239-container-reconciliation: (184 commits) HDDS-10373. Implement framework for capturing Merkle Tree Metrics. (apache#6864) HDDS-11188. Initial setup for new UI layout and enable users to switch to new UI (apache#6953) HDDS-11120. Rich rebalancing status info (apache#6911) HDDS-11187. Fix Event Handling in Recon OMDBUpdatesHandler to Prevent ClassCastException. (apache#6950) HDDS-11213. Bump commons-daemon to 1.4.0 (apache#6971) HDDS-11212. Bump commons-net to 3.11.1 (apache#6973) HDDS-11211. Bump assertj-core to 3.26.3 (apache#6972) HDDS-11210. Bump log4j2 to 2.23.1 (apache#6970) HDDS-11150. Recon Overview page crashes due to failed API Calls (apache#6944) HDDS-11183. Keys from DeletedTable and DeletedDirTable of AOS should be deleted on batch operation while creating a snapshot (apache#6946) HDDS-11198. Fix Typescript configs for Recon (apache#6961) HDDS-11180. Simplify HttpServer2#inferMimeType return statement (apache#6963) HDDS-11194. OM missing audit log for upgrade (apache#6958) HDDS-10389. Implement a search feature for users to locate open keys within the Open Keys Insights section. (apache#6231) HDDS-10561. Dashboard for delete key metrics (apache#6948) HDDS-11192. Increase SPNEGO URL test coverage (apache#6956) HDDS-11179. DBConfigFromFile#readFromFile result of toIOException not thrown (apache#6957) HDDS-11186. First container log missing from bundle (apache#6952) HDDS-10844. Clarify snapshot create error message. (apache#6955) HDDS-11166. Switch to Rocky Linux-based ozone-runner (apache#6942) ...
What changes were proposed in this pull request?
The existing search functionality for open keys in the Open Keys Insights section is limited to the dataset returned by the API endpoint, which only provides a subset of the available data. It is necessary to enhance this search feature to ensure users can accurately find the specific key information they are seeking or the closest available match.
I have implemented a new search endpoint for Recon Insights specifically for open keys:
Currently, this feature is limited to
open keysto keep the initial implementation manageable and prevent the patch from becoming overly large. Since the search functionality involves querying both the KeyTable and the FileTable, we decided to introduce the feature for open keys first. The inclusion of search capabilities for deleted keys and directories will be addressed in subsequent PR's.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10389
How was this patch tested?
Unit Testing and Manual Testing