make http runner available as library, take 2 by ldemailly · Pull Request #520 · istio/istio

ldemailly · 2017-08-01T03:01:29Z

Moved a lot of the code from fortio main to the library so #517 can be
better/easier

(This is same as #519 with attempt at preserving history and without
the grpc wip)

Moved a lot of the code from fortio main to the library so #517 can be better/easier (This is same as #519 with attempt at preserving history and without the grpc wip)

ldemailly · 2017-08-01T03:03:23Z

@douglas-reid this version should be easier to review (even though github is not smart enough to notice the origin of the new file and show diffs...)

ldemailly · 2017-08-01T03:05:27Z

this is the view that has the most readable diff
a1f87a3#diff-1960f6ed47cfa60dd75b0f30a997add9

douglas-reid

Looks pretty good. Just a few late-night naming questions, mainly centered around matching some of the conventions with http.Transport.

douglas-reid · 2017-08-01T05:33:35Z

devel/fortio/httprunner.go

+}
+
+// HTTPRunner runs an http test and returns the aggregated stats.
+func HTTPRunner(o *HTTPRunnerOptions) (*HTTPRunnerResults, error) {


Since this actually causes the Runner to Run, should we call this something like RunHTTP or RunHTTPTest?

douglas-reid · 2017-08-01T05:46:22Z

devel/fortio/httprunner.go

+type HTTPRunnerOptions struct {
+	RunnerOptions
+	URL         string
+	Compression bool   // defaults to no compression, only used by std client


Any reason to default to no compression? Could this be DisableCompression ?

yes to lower cpu (even though it doesn't really apply when there is no payload)

douglas-reid · 2017-08-01T05:47:10Z

devel/fortio/httprunner.go

+	RunnerOptions
+	URL         string
+	Compression bool   // defaults to no compression, only used by std client
+	StdClient   bool   // defaults to fast client


maybe DisableFastClient ?

ok, changed

douglas-reid · 2017-08-01T05:47:29Z

devel/fortio/httprunner.go

+	Compression bool   // defaults to no compression, only used by std client
+	StdClient   bool   // defaults to fast client
+	HTTP10      bool   // defaults to http1.1
+	NoKeepAlive bool   // so default is keep alive


DisableKeepAlives?

changed (without s)

douglas-reid · 2017-08-01T05:51:59Z

devel/fortio/httprunner.go

+	URL         string
+	Compression bool   // defaults to no compression, only used by std client
+	StdClient   bool   // defaults to fast client
+	HTTP10      bool   // defaults to http1.1


I might be tempted to use an enum here (in the future, you may want to add an http/2 client).

will see then, not sure how I'll add grpc yet

douglas-reid · 2017-08-01T06:00:02Z

devel/fortio/httprunner.go

+
+// TestHTTP http request fetching. Main call being run at the target QPS.
+// To be set as the Function in RunnerOptions.
+func TestHTTP(t int) {


Maybe HTTPGet (or HTTPFetch)?

updated comment

douglas-reid · 2017-08-01T06:01:30Z

devel/fortio/httprunner.go

+
+package fortio
+
+import (


do we need a httprunner_test.go for this new library-ification?

istio-merge-robot · 2017-08-01T16:56:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ldemailly

Associated issue: 517

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

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [ldemailly]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@douglas-reid

Added test, moved echosrv code to lib, created DynamicHTTP for test, renames per @douglas-reid code review comments

istio-testing · 2017-08-01T19:07:06Z

Jenkins job istio/presubmit passed

istio-testing · 2017-08-01T19:10:55Z

Jenkins job istio/presubmit passed

istio-testing · 2017-08-01T19:16:49Z

Jenkins job istio/presubmit passed

istio-testing · 2017-08-01T19:16:51Z

@ldemailly: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-presubmit.sh	`ebcdf97`	link	`@istio-testing bazel test this`

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ldemailly · 2017-08-02T05:06:45Z

Jenkins passed and I had talked to Doug (and also got confused by the "approved" label) so I pushed.
thx seb for having fixed the linter issue in a945362

@douglas-reid

* make http runner available as library Moved a lot of the code from fortio main to the library so #517 can be better/easier (This is same as #519 with attempt at preserving history and without the grpc wip) * Added test, moved echosrv code to lib, ... Added test, moved echosrv code to lib, created DynamicHTTP for test, renames per @douglas-reid code review comments * Better initialization syntax * Add a negative testing Former-commit-id: 7b83910

@douglas-reid

* make http runner available as library Moved a lot of the code from fortio main to the library so istio#517 can be better/easier (This is same as istio#519 with attempt at preserving history and without the grpc wip) * Added test, moved echosrv code to lib, ... Added test, moved echosrv code to lib, created DynamicHTTP for test, renames per @douglas-reid code review comments * Better initialization syntax * Add a negative testing Former-commit-id: 7b83910

@douglas-reid

* make http runner available as library Moved a lot of the code from fortio main to the library so #517 can be better/easier (This is same as #519 with attempt at preserving history and without the grpc wip) * Added test, moved echosrv code to lib, ... Added test, moved echosrv code to lib, created DynamicHTTP for test, renames per @douglas-reid code review comments * Better initialization syntax * Add a negative testing Former-commit-id: 7b83910

* Add integration tests. * add fault_inject_test * format * Update comment. * fix format.

Co-authored-by: maistra-bot <null>

Co-authored-by: openshift-service-mesh-bot <null>

ldemailly added 2 commits July 31, 2017 19:48

cp code from fortio main to httprunner.go

3dccd62

make http runner available as library

a1f87a3

Moved a lot of the code from fortio main to the library so #517 can be better/easier (This is same as #519 with attempt at preserving history and without the grpc wip)

googlebot added the cla: yes label Aug 1, 2017

ldemailly requested a review from douglas-reid August 1, 2017 03:02

ldemailly mentioned this pull request Aug 1, 2017

grpc support in fortio (grpc load testing) #519

Merged

douglas-reid reviewed Aug 1, 2017

View reviewed changes

istio-merge-robot added the approved label Aug 1, 2017

ldemailly added 3 commits August 1, 2017 11:38

Added test, moved echosrv code to lib, ...

073c1a6

Added test, moved echosrv code to lib, created DynamicHTTP for test, renames per @douglas-reid code review comments

Merge branch 'master' into httprunner

0e22bde

Better initialization syntax

850c092

Add a negative testing

ebcdf97

ldemailly merged commit 7b83910 into master Aug 1, 2017

ldemailly deleted the httprunner branch August 1, 2017 19:16

kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018

Add some integration tests (istio#520)

5d54418

* Add integration tests. * add fault_inject_test * format * Update comment. * fix format.

luksa pushed a commit to luksa/istio that referenced this pull request Sep 20, 2022

Automator: Update proxy (istio#520)

78fe402

Co-authored-by: maistra-bot <null>

cam-garrison pushed a commit to cam-garrison/istio that referenced this pull request Oct 29, 2025

[release-1.27] Automator: Update proxy (istio#520)

41b4fc0

Co-authored-by: openshift-service-mesh-bot <null>


		package fortio

		import (

Conversation

ldemailly commented Aug 1, 2017

Uh oh!

ldemailly commented Aug 1, 2017

Uh oh!

ldemailly commented Aug 1, 2017

Uh oh!

douglas-reid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Aug 1, 2017

Uh oh!

istio-testing commented Aug 1, 2017

Uh oh!

istio-testing commented Aug 1, 2017

Uh oh!

istio-testing commented Aug 1, 2017

Uh oh!

istio-testing commented Aug 1, 2017

Uh oh!

ldemailly commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldemailly commented Aug 2, 2017 •

edited

Loading