http.Agent: Add `Group` variants for parallel fetching #107

puerco · 2024-07-19T23:57:39Z

What type of PR is this?

/kind feature
/kind documentation

What this PR does / why we need it:

This pull request adds *Group variants of the existing http.Agent methods to support parallel fetching of multiple URLs. The new included docs explain it better:

Each of these functions also provide a _Group_ equivalent that takes a list of URLs and performs the requests in parallel. The easiest way to understand the functions is this expression:

METHOD[Request|ToWriter][Group]

So, for examaple, the functions for the POST method include the following variations, note that the Group
variants take and return the same arguments but in plural form, ie same type but a slice:

 Post(string url, []byte postData) ([]byte, error)
 PostRequest(string url, []byte postData) (*http.Response, error)
 PostToWriter(io.Writer w, string url, []byte postData) error
 PostGroup([]string urls, [][]byte postData) ([][]byte, []error)
 PostRequestGroup([]string urls, [][]byte postData) ([]*http.Response, []error)
 PostToWriterGroup([]io.Writer w, []string urls, [][]byte postData) []error

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Most of the line changes in the PR is documentation, the real code is in 482d365

I added a doc.go to document the module better and an example to illustrate the new functions.

The functions rely on the existing fetch logic, I added a test to check that the parallel logic returns values in the expected order.

Finally I marked as deprecated the http.GetURLResponse() function to encourage developers to use the agent directly.

Does this PR introduce a user-facing change?

The `http.Agent` now has `*Group` variants of its functions to support parallel fetching o lists of URLs.

cpanato · 2024-07-20T07:04:10Z

http/agent.go

@@ -74,6 +77,7 @@ var defaultAgentOptions = &agentOptions{
 	Timeout:         3 * time.Second,
 	MaxWaitTime:     60 * time.Second,
 	PostContentType: defaultPostContentType,
+	MaxParallel:     5,


As a follow-up, can we make this configurable somehow?

You can configure it with the config functions. I added one for MaxParallel, for example:

a := http.NewAgent().WithMaxParallel(10)

cpanato

looks cool, just a few nit/questions

cpanato · 2024-07-20T07:04:50Z

http/example_multi_test.go

+	"sigs.k8s.io/release-utils/http"
+)
+
+func Example() {


can we have a better test name?

Mmh any suggestions? It is supposed to be a general example, no tied to a function so I was following the "Whole file example" convention (see the go examples documentation)

I'm not sure I'm a huge fan of having examples in the same package as the code in general. What about having a dedicated package at the repo root (e.g. examples) where we can put code examples for various different packages in this repo?

Ah ok, I can remove it. This is so that it shows on the go documentation, such as this:
https://pkg.go.dev/strings#example-Builder

cpanato · 2024-07-20T07:05:57Z

http/example_multi_test.go

+	agent := http.NewAgent()
+	w := []io.Writer{}
+	urls := []string{
+		"https://live.staticflickr.com/65535/53863838503_3490725fab.jpg",


Should we use other links, or is this okay, and will it not disappear?

They could dissapear, but since it's just an example (for the documentation) I'm not too concerned.

go.mod

puerco · 2024-07-23T07:55:06Z

OK, I've replaced the external dependency with the new throttle package and repushed. PTAL!

puerco · 2024-07-23T07:55:17Z

OK, I've replaced the external dependency with the new throttle package and repushed. PTAL!

xmudrii · 2024-07-23T11:11:39Z

http/agent.go

+// The list of URLs and postData byte arrays are expected to be of equal length.
+// If postData has less elements than the url list, those urls without a corresponding
+// postData array will return an error.


What's the reason for not failing as early as possible, given that the function will eventually fail anyways?

What I was trying to do was to do all requests that were plausible but that introduced an inconsistent behavior. I took your advice here and now the function will fail early if URLs and postdata don't line up. I also updated the test to check for it.

http/agent.go

This commit addds Group Variants to the agent functions to fetch files in parallel. Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

This commit adds a doc.go with documentation about the module and an example showing how to do parallel feching with the agent. Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

This xommit updates the http tests to avoid retries to speed them up. Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

puerco · 2024-07-23T21:40:31Z

OK, I pushed another commit optimizing some of the tests, should be ready to go now.

# Before

[puerco@babieco release-utils] 🦆❯ time  go test ./http/...
?   	sigs.k8s.io/release-utils/http/httpfakes	[no test files]
ok  	sigs.k8s.io/release-utils/http	18.407s

real	0m18.660s
user	0m0.456s
sys	0m0.176s


# After

[puerco@babieco release-utils] on  parallel-http 🦆❯ time  go test ./http/...
?   	sigs.k8s.io/release-utils/http/httpfakes	[no test files]
ok  	sigs.k8s.io/release-utils/http	0.264s

real	0m0.522s
user	0m0.487s
sys	0m0.158s

k8s-ci-robot · 2024-07-24T16:16:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, puerco

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cpanato,puerco]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/documentation Categorizes issue or PR as related to documentation. labels Jul 19, 2024

k8s-ci-robot requested a review from jrsapi July 19, 2024 23:57

k8s-ci-robot added the sig/release Categorizes an issue or PR as relevant to SIG Release. label Jul 19, 2024

k8s-ci-robot requested a review from Verolop July 19, 2024 23:57

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 19, 2024

cpanato reviewed Jul 20, 2024

View reviewed changes

cpanato requested changes Jul 20, 2024

View reviewed changes

k8s-ci-robot assigned cpanato Jul 20, 2024

saschagrunert reviewed Jul 22, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

puerco mentioned this pull request Jul 23, 2024

Add new throttler pkg forked from nozzle/throttler #108

Merged

puerco force-pushed the parallel-http branch 2 times, most recently from 8d37564 to 1cc97f7 Compare July 23, 2024 07:47

xmudrii reviewed Jul 23, 2024

View reviewed changes

puerco force-pushed the parallel-http branch from 1cc97f7 to 548ff2a Compare July 23, 2024 21:00

puerco added 5 commits July 23, 2024 15:06

http agent: Add Group Variants

3710696

This commit addds Group Variants to the agent functions to fetch files in parallel. Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

http agent: Add tests for parallel fetching

abf057c

Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

HTTP Group: Add documentation and example

1cd03a3

This commit adds a doc.go with documentation about the module and an example showing how to do parallel feching with the agent. Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

Mark http.GetURLResponse as deprecated

9c49082

Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

Improve post tests to actually check returns

18dab2e

Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

puerco force-pushed the parallel-http branch from 548ff2a to 18dab2e Compare July 23, 2024 21:06

Parallelize tests and do not retry

b6576e7

This xommit updates the http tests to avoid retries to speed them up. Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>

cpanato approved these changes Jul 24, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2024

k8s-ci-robot merged commit 61086f6 into kubernetes-sigs:main Jul 24, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http.Agent: Add `Group` variants for parallel fetching #107

http.Agent: Add `Group` variants for parallel fetching #107

puerco commented Jul 19, 2024 •

edited

Loading

cpanato Jul 20, 2024

puerco Jul 22, 2024

cpanato left a comment

cpanato Jul 20, 2024

puerco Jul 22, 2024

xmudrii Jul 23, 2024

puerco Jul 24, 2024

cpanato Jul 20, 2024

puerco Jul 22, 2024 •

edited

Loading

puerco commented Jul 23, 2024

puerco commented Jul 23, 2024

xmudrii Jul 23, 2024

puerco Jul 23, 2024

puerco commented Jul 23, 2024

k8s-ci-robot commented Jul 24, 2024

http.Agent: Add Group variants for parallel fetching #107

http.Agent: Add Group variants for parallel fetching #107

Conversation

puerco commented Jul 19, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpanato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puerco Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

puerco commented Jul 23, 2024

puerco commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puerco commented Jul 23, 2024

k8s-ci-robot commented Jul 24, 2024

http.Agent: Add `Group` variants for parallel fetching #107

http.Agent: Add `Group` variants for parallel fetching #107

puerco commented Jul 19, 2024 •

edited

Loading

puerco Jul 22, 2024 •

edited

Loading