[v8] Make retrieval of tasks in bulk instead of small increments #3276
Conversation
api/cloudcontroller/ccv3/paginate.go
Outdated
| @@ -69,3 +64,25 @@ func (requester RealRequester) wrapFirstPage(request *cloudcontroller.Request, o | |||
|
|
|||
| return wrapper, warnings, nil | |||
| } | |||
|
|
|||
| func (requester RealRequester) bulkRetrieval(request *cloudcontroller.Request, obj interface{}, appendToExternalList func(interface{}) error) (IncludedResources, Warnings, error) { | |||
There was a problem hiding this comment.
question Do we need this at all? Can we just do
query := []ccv3.Query{{Key: ccv3.PerPage, Values: []string{ccv3.MaxPerPage}}}
and pass it through here and use the existing pagination methods?
There was a problem hiding this comment.
We could but I am trying to make it more generic and only apply to the commands that we want
There was a problem hiding this comment.
I'm probably not understanding, what is the difference here between the new bulkRetrieval function and and an specific actor setting MaxPerPage like here and calling the existing paginate function?
There was a problem hiding this comment.
Now I see what you mean.
In the end, I do not have a strong preference here it felt more natural to abstract this into the requests than to have this logic scattered around every action, but I also see the argument of reusability of the paginate function, specially because of the strange interface of that function that ignores outputs of the requests......
If you want I can make the change as you mentioned.
Signed-off-by: João Pereira <[email protected]>
- Fix issue, where code was always doing 2 requests even when it was able to get all the tasks in the first request Signed-off-by: João Pereira <[email protected]>
Signed-off-by: João Pereira <[email protected]>
Signed-off-by: João Pereira <[email protected]>
joaopapereira
force-pushed
the
bulk-retrieval-of-tasks-v8
branch
from
9b6e402
to
4fc05d2
Compare
| @@ -12,3 +12,14 @@ type IncludedResources struct { | |||
| ServicePlans []resources.ServicePlan `json:"service_plans,omitempty"` | |||
| Apps []resources.Application `json:"apps,omitempty"` | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
suggestion Perhaps including a small test result showing the difference in time for cf tasks between the old and new behaviour would be useful? Maybe with an app with like 10,000 tasks?
Signed-off-by: João Pereira <[email protected]>
Description of the Change
In this PR we change the way tasks are retrieved in CLI to lower the load that it currently has in CAPI when there are a big number of tasks associated with a particular application
Related to #3274