Skip to content
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

[v8] Make retrieval of tasks in bulk instead of small increments #3276

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Make retrieval of tasks in bulk instead of small increments
Signed-off-by: João Pereira <[email protected]>
joaopapereira committed Nov 5, 2024
commit 22234b45812255e1b81fa3182a914cb24c84ef8e
26 changes: 26 additions & 0 deletions api/cloudcontroller/ccv3/paginate.go
Original file line number Diff line number Diff line change
@@ -2,10 +2,15 @@ package ccv3

import (
"net/http"
"net/url"
"strconv"
"strings"

"code.cloudfoundry.org/cli/api/cloudcontroller"
)

const MaxResultsPerPage int = 5000
Samze marked this conversation as resolved.
Show resolved Hide resolved

func (requester RealRequester) paginate(request *cloudcontroller.Request, obj interface{}, appendToExternalList func(interface{}) error, specificPage bool) (IncludedResources, Warnings, error) {
fullWarningsList := Warnings{}
var includes IncludedResources
@@ -69,3 +74,24 @@ 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) {
Copy link
Contributor

@Samze Samze Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could but I am trying to make it more generic and only apply to the commands that we want

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

wrapper, warnings, err := requester.wrapFirstPage(request, obj, func(_ interface{}) error { return nil })
if err != nil {
return IncludedResources{}, warnings, err
}

newQuery := url.Values{}
for name, value := range request.URL.Query() {
if name != "" && name != string(PerPage) {
newQuery.Add(name, strings.Join(value, ","))
}
}
resultsPerPage := strconv.Itoa(wrapper.Pagination.TotalResults)
Samze marked this conversation as resolved.
Show resolved Hide resolved
if wrapper.Pagination.TotalResults > MaxResultsPerPage {
resultsPerPage = strconv.Itoa(MaxResultsPerPage)
}
newQuery.Add(string(PerPage), resultsPerPage)
request.URL.RawQuery = newQuery.Encode()
return requester.paginate(request, obj, appendToExternalList, false)
}
2 changes: 2 additions & 0 deletions api/cloudcontroller/ccv3/paginated_resources.go
Original file line number Diff line number Diff line change
@@ -23,6 +23,8 @@ type PaginatedResources struct {
// HREF is the HREF of the next page.
HREF string `json:"href"`
} `json:"next"`
TotalResults int `json:"total_results"`
TotalPages int `json:"total_pages"`
} `json:"pagination"`
// ResourceBytes is the list of resources for the current page.
ResourcesBytes json.RawMessage `json:"resources"`
19 changes: 12 additions & 7 deletions api/cloudcontroller/ccv3/requester.go
Original file line number Diff line number Diff line change
@@ -15,13 +15,14 @@ import (
//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Requester

type RequestParams struct {
RequestName string
URIParams internal.Params
Query []Query
RequestBody interface{}
ResponseBody interface{}
URL string
AppendToList func(item interface{}) error
RequestName string
URIParams internal.Params
Query []Query
RequestBody interface{}
ResponseBody interface{}
URL string
AppendToList func(item interface{}) error
BulkIfPossible bool
}

type Requester interface {
@@ -95,6 +96,10 @@ func (requester *RealRequester) MakeListRequest(requestParams RequestParams) (In
return IncludedResources{}, nil, err
}

if requestParams.BulkIfPossible {
return requester.bulkRetrieval(request, requestParams.ResponseBody, requestParams.AppendToList)
}

specificPage := false
for _, query := range requestParams.Query {
if query.Key == Page {
1 change: 1 addition & 0 deletions api/cloudcontroller/ccv3/task.go
Original file line number Diff line number Diff line change
@@ -34,6 +34,7 @@ func (client *Client) GetApplicationTasks(appGUID string, query ...Query) ([]res
tasks = append(tasks, item.(resources.Task))
return nil
},
BulkIfPossible: true,
})

return tasks, warnings, err
198 changes: 195 additions & 3 deletions api/cloudcontroller/ccv3/task_test.go
Original file line number Diff line number Diff line change
@@ -201,7 +201,8 @@ var _ = Describe("Task", func() {
"pagination": {
"next": {
"href": "%s/v3/apps/some-app-guid/tasks?per_page=2&page=2"
}
},
"total_results": 3
},
"resources": [
{
@@ -227,6 +228,22 @@ var _ = Describe("Task", func() {
"next": null
},
"resources": [
{
"guid": "task-1-guid",
"sequence_id": 1,
"name": "task-1",
"command": "some-command",
"state": "SUCCEEDED",
"created_at": "2016-11-07T05:59:01Z"
},
{
"guid": "task-2-guid",
"sequence_id": 2,
"name": "task-2",
"command": "some-command",
"state": "FAILED",
"created_at": "2016-11-07T06:59:01Z"
},
{
"guid": "task-3-guid",
"sequence_id": 3,
@@ -245,7 +262,7 @@ var _ = Describe("Task", func() {
)
server.AppendHandlers(
CombineHandlers(
VerifyRequest(http.MethodGet, "/v3/apps/some-app-guid/tasks", "per_page=2&page=2"),
VerifyRequest(http.MethodGet, "/v3/apps/some-app-guid/tasks", "per_page=3"),
RespondWith(http.StatusOK, response2, http.Header{"X-Cf-Warnings": {"warning-2"}}),
),
)
@@ -282,7 +299,7 @@ var _ = Describe("Task", func() {
Command: "some-command",
},
))
Expect(warnings).To(ConsistOf("warning-1", "warning-2"))
Expect(warnings).To(ConsistOf("warning-2"))
})
})

@@ -353,6 +370,181 @@ var _ = Describe("Task", func() {
Expect(warnings).To(ConsistOf("warning"))
})
})

When("the application has 10000 tasks", func() {
BeforeEach(func() {
response1 := fmt.Sprintf(`{
"pagination": {
"next": {
"href": "%s/v3/apps/some-app-guid/tasks?per_page=2&page=2"
},
"total_results": 10000
},
"resources": [
{
"guid": "task-1-guid",
"sequence_id": 1,
"name": "task-1",
"command": "some-command",
"state": "SUCCEEDED",
"created_at": "2016-11-07T05:59:01Z"
},
{
"guid": "task-2-guid",
"sequence_id": 2,
"name": "task-2",
"command": "some-command",
"state": "FAILED",
"created_at": "2016-11-07T06:59:01Z"
}
]
}`, server.URL())

response2 := fmt.Sprintf(`{
"pagination": {
"next": {
"href": "%s/v3/apps/some-app-guid/tasks?per_page=5000&page=2"
},
"total_results": 10000
},
"resources": [
{
"guid": "task-1-guid",
"sequence_id": 1,
"name": "task-1",
"command": "some-command",
"state": "SUCCEEDED",
"created_at": "2016-11-07T05:59:01Z"
},
{
"guid": "task-2-guid",
"sequence_id": 2,
"name": "task-2",
"command": "some-command",
"state": "FAILED",
"created_at": "2016-11-07T06:59:01Z"
}
]
}`, server.URL())
response3 := fmt.Sprintf(`{
"pagination": {
"total_results": 10000
},
"resources": [
{
"guid": "task-1-guid",
"sequence_id": 1,
"name": "task-1",
"command": "some-command",
"state": "SUCCEEDED",
"created_at": "2016-11-07T05:59:01Z"
},
{
"guid": "task-2-guid",
"sequence_id": 2,
"name": "task-2",
"command": "some-command",
"state": "FAILED",
"created_at": "2016-11-07T06:59:01Z"
}
]
}`)

server.AppendHandlers(
CombineHandlers(
VerifyRequest(http.MethodGet, "/v3/apps/some-app-guid/tasks"),
RespondWith(http.StatusAccepted, response1, http.Header{"X-Cf-Warnings": {"warning"}}),
),
)
server.AppendHandlers(
CombineHandlers(
VerifyRequest(http.MethodGet, "/v3/apps/some-app-guid/tasks", "per_page=5000"),
RespondWith(http.StatusOK, response2, http.Header{"X-Cf-Warnings": {"warning-2"}}),
),
)
server.AppendHandlers(
CombineHandlers(
VerifyRequest(http.MethodGet, "/v3/apps/some-app-guid/tasks", "page=2&per_page=5000"),
RespondWith(http.StatusOK, response3, http.Header{"X-Cf-Warnings": {"warning-2"}}),
),
)
})

It("calls CAPI 3 times", func() {
Expect(executeErr).ToNot(HaveOccurred())
})
})

When("the application has 4999 tasks", func() {
BeforeEach(func() {
response1 := fmt.Sprintf(`{
"pagination": {
"next": {
"href": "%s/v3/apps/some-app-guid/tasks?per_page=2&page=2"
},
"total_results": 4999
},
"resources": [
{
"guid": "task-1-guid",
"sequence_id": 1,
"name": "task-1",
"command": "some-command",
"state": "SUCCEEDED",
"created_at": "2016-11-07T05:59:01Z"
},
{
"guid": "task-2-guid",
"sequence_id": 2,
"name": "task-2",
"command": "some-command",
"state": "FAILED",
"created_at": "2016-11-07T06:59:01Z"
}
]
}`, server.URL())

response2 := fmt.Sprintf(`{
"pagination": {
"total_results": 4999
},
"resources": [
{
"guid": "task-1-guid",
"sequence_id": 1,
"name": "task-1",
"command": "some-command",
"state": "SUCCEEDED",
"created_at": "2016-11-07T05:59:01Z"
},
{
"guid": "task-2-guid",
"sequence_id": 2,
"name": "task-2",
"command": "some-command",
"state": "FAILED",
"created_at": "2016-11-07T06:59:01Z"
}
]
}`)
server.AppendHandlers(
CombineHandlers(
VerifyRequest(http.MethodGet, "/v3/apps/some-app-guid/tasks"),
RespondWith(http.StatusAccepted, response1, http.Header{"X-Cf-Warnings": {"warning"}}),
),
)
server.AppendHandlers(
CombineHandlers(
VerifyRequest(http.MethodGet, "/v3/apps/some-app-guid/tasks", "per_page=4999"),
RespondWith(http.StatusOK, response2, http.Header{"X-Cf-Warnings": {"warning-2"}}),
),
)
})

It("calls CAPI 2 times", func() {
Expect(executeErr).ToNot(HaveOccurred())
})
})
})

Describe("UpdateTaskCancel", func() {