-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-12708. Fix Unhealthy Containers API for pagination #8796
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
Change-Id: I3aee1631a262e2d941dbbbcddcb7a7c5e286d33a
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 the patch. Few nits.
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java
Outdated
Show resolved
Hide resolved
...recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/UnhealthyContainersResponse.java
Show resolved
Hide resolved
...recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/UnhealthyContainersResponse.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 the patch. Current impl looks good. But can you pls update the problem or issue with existing current implementation in your PR description ?
What was the problem earlier?The unhealthy containers API had broken pagination - it only worked in one direction (forward). The issue:
Why this was bad:
How did we solve it?We added bidirectional pagination by introducing two new parameters:
Explanation with Example:Before (broken):
After (fixed): Real-world scenario:
The fix:
Result:
In simple terms: We fixed the pagination so users can go both ways instead of being stuck going only forward. |
Pls update the same in PR description with UI screenshot. |
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 API testing and providing explanation. Kindly put the same in PR description , what was the issue in previous approach and what we are fixing in this PR. If you want, you can remove from comments, Also kindly mention the UI change PR for this backend change, here , so that there is clarity.
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.
Also changing the API signature where we don't have now default batch number, can you test with existing UI code ?
devabhishekpal
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 this patch.
LGTM, +1
|
Patch for the UI changes - #8862 |
Thanks @ArafatKhan2198 for the revised changes and thanks @devabhishekpal for the review. Changes LGTM +1 |
What changes were proposed in this pull request?
Problem Statement
The unhealthy containers API had broken pagination - it only supported forward navigation, making it impossible for users to go back to previous pages without starting over from the beginning.
Root Cause
prevKeyparameter for forward paginationSolution Implemented
Added bidirectional pagination with two new parameters:
New Parameters:
maxContainerId- for backward pagination (Previous button)minContainerId- for forward pagination (Next button)Technical Changes:
firstKey/lastKeyfor paginationBefore vs After
Before (Broken):
After (Fixed):
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12708
How was this patch tested?
Bidirectional Pagination Test Report for Unhealthy Containers API
Initial Dataset Overview
Total Missing Containers: 9 containers
Container IDs in ascending order: [2, 4, 6, 8, 12, 14, 16, 18, 20]
Test Results
Test 1: First Page (Baseline)
Endpoint:
GET http://localhost:9888/api/v1/containers/unhealthy/MISSING?limit=3Response Summary:
Test 2: Forward Pagination (Next Page)
Endpoint:
GET http://localhost:9888/api/v1/containers/unhealthy/MISSING?limit=3&minContainerId=6Response Summary:
Test 3: Forward Pagination (Next Page)
Endpoint:
GET http://localhost:9888/api/v1/containers/unhealthy/MISSING?limit=3&minContainerId=14Response Summary:
Test 4: Backward Pagination (Previous Page)
Endpoint:
GET http://localhost:9888/api/v1/containers/unhealthy/MISSING?limit=3&maxContainerId=16Response Summary:
Navigation Flow Verification
Forward Navigation Sequence:
minContainerId=6)minContainerId=14)Backward Navigation Sequence:
maxContainerId=16)maxContainerId=8)Conclusion
✅ BIDIRECTIONAL PAGINATION IS WORKING CORRECTLY
The API successfully demonstrates:
minContainerIdparametermaxContainerIdparameterfirstKeyandlastKeyvaluesThe implementation successfully addresses the original problem of limited pagination support and now provides full bidirectional navigation capabilities for the unhealthy containers API.