-
Notifications
You must be signed in to change notification settings - Fork 60
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
[RHCLOUD-36102] - Slow loading of data from roles endpoint perfo… #1354
[RHCLOUD-36102] - Slow loading of data from roles endpoint perfo… #1354
Conversation
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.
Based on the extra query you got from django silk, this seems to be n+1 query issue.
I think you can improve the performance by using prefetch related in the get_role_queryset method. We just need to include the permission lookup and external relation lookup there as well.
…ynamically when passed in add_fields query param
/retest |
1 similar comment
/retest |
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! Not sure if the default limit would change any behavior though
rbac/management/role/view.py
Outdated
@@ -141,6 +142,7 @@ class RoleViewSet( | |||
filterset_class = RoleFilter | |||
ordering_fields = ("name", "display_name", "modified", "policyCount") | |||
ordering = ("name",) | |||
default_limit = StandardResultsSetPagination.default_limit |
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.
Is this intended?
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.
Nope thats redundant removed now, was from my testing of filtering and pagination stuff
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! Not sure if the default limit would change any behavior though. Need a second eye
…rmance fixes
Link(s) to Jira
https://issues.redhat.com/browse/RHCLOUD-36102
Description of Intent of Change(s)
Originally focused on fine grained handling of filtering and pagination, this did not lead to any significant reduction in number of queries or performance improvements.
Using prefetch_related for "access", "ext_relation" and "access__permission" reduced the number of queries from 33 to 6 for a request to the roles endpoint for the get_role_queryset method.
Local Testing
How can the feature be exercised?
How can the bug be exploited and fix confirmed?
Is any special local setup required?
Checklist
Secure Coding Practices Checklist Link
Secure Coding Practices Checklist