listNetworks optimization #9096
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #9096 +/- ##
============================================
- Coverage 14.96% 4.31% -10.65%
============================================
Files 5373 363 -5010
Lines 469024 29180 -439844
Branches 58818 5091 -53727
============================================
- Hits 70197 1260 -68937
+ Misses 391056 27778 -363278
+ Partials 7771 142 -7629
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -2267,7 +2267,7 @@ public Pair<List<? extends Network>, Integer> searchForNetworks(ListNetworksCmd | |||
| isRecursive = true; | |||
| } | |||
|
|
|||
| Filter searchFilter = new Filter(NetworkVO.class, "id", false, null, null); | |||
| Filter searchFilter = new Filter(NetworkVO.class, "id", false, cmd.getStartIndex(), cmd.getPageSizeVal()); | |||
There was a problem hiding this comment.
if networkfilter is Account, Domain or Shared, it should work
However, if the networkfilter is All or AccountDomain, it might not work if the searchFilter is the same
for example, there are 25 networks for account, 25 networks for domain. if networkfilter is All or AccountDomain, and the page size is 20,
- on first page, it returns 20 networks for account, it is good
- on second page, it should return the last 5 networks for account, and the first 15 networks for domain. But it will return the last 5 networks for account, and the last 5 networks for domain. right ?
I think thesearchFilterneeds to be updated after the each search.
There was a problem hiding this comment.
That's a very good point.
The issue here is that we are concatenating results from multiple queries ( lest's say Q1, Q2, Q3). Things kind of work for the first page. We can adjust pageSize of a query depending on how many results are already returned for other queries.
But if we want to display the second page, it is hard to say which startIndex should be used for Q2. As it depends on how many results were displayed for Q2 on page 1. Which in turn depends on how many Q1 results were there on Page 1. For example, if page 1 had 15 results for Q1 and 5 results for Q2, then page2 Q2 startIndex should be 6.
So, to fix this I think the filter should always have startIndex as 0 and pageSize should be cmd.getPageSizeVal() * (cmd.getStartIndex() + 1)
Filter searchFilter = new Filter(NetworkVO.class, "id", false, 0, (cmd.getStartIndex() + 1) * cmd.getPageSizeVal());
This way we have info on all previous page results and when pagination is applied at the end of the method, the correct result will be returned.
We are losing some optimization if startIndex is higher but it is still better than reading all rows.
There was a problem hiding this comment.
Are there not any other API similar to this where pagination is applied where the pattern can be repeated, maybe templates, volumes, offerings?
| aclType, true, restartRequired, specifyIpRanges, vpcId, tags, display, vlanId, associatedNetworkId), searchFilter, permittedAccounts); | ||
| addNetworksToReturnIfNotExist(networksToReturn, sharedNetworks); | ||
| addNetworksToReturnIfNotExist(networksToReturn, sharedNetworks.first()); |
There was a problem hiding this comment.
The total count of result won't be super accurate now.
As before we would have gone through all the account specific and domain level networks and gotten the union with sharedNetworks. That would have returned the exact total networks count.
But now we will be working with at max 20 account and domain level networks in the list. So there is no way to apply union with all the shared networks.
For example,
Domain level netowrks = 100
Shared networks = 100
Intersection(Domain, Shared) among all of the domain and shared networks = 50
Before this change :
Total network count = Domain n/w + shared n/w - intersection = 150
After this change:
Intersection(Domain, shared) among the first 20 domain and shared networks returned by the query = 10
Total network count = Domain n/w + shared n/w - intersection = 190
There was a problem hiding this comment.
This needs to be accurate too. Perhaps once the query returns the page, we can call a dao::getCount() passing only the list of IDs?
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9669 |
|
Discussed with @abh1sar. It will be worth investigating if instead of querying networks multiple times for different filters. |
|
any update/progress on this @shwstppr ? |
@rohityadavcloud, |
|
@rohityadavcloud @abh1sar cc @weizhouapache I created a draft here #9184 to refactor the code to retrieve networks from DB only once. I'm yet to test its efficacy for different parameters but if it works it should allow listing networks with a single DB query |
Description
This PR fixes the performance issue where listNetworks api goes through all the rows of the networks table and takes longer irrespective of the pagination used.
[Draft PR. Needs discussion around how the count of networks is displayed.]
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?