kncloudevents now uses a singleton http client #4465

slinkydeveloper · 2020-11-05T08:55:36Z

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Proposed Changes

Now every new message sender always reuse the same underlying client, whenever possible

Now every new message sender always reuse the same underlying client, whenever possible Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper · 2020-11-05T08:58:58Z

This is an important fix, do we want it before or after the release?

codecov · 2020-11-05T09:02:06Z

Codecov Report

Merging #4465 into master will increase coverage by 0.06%.
The diff coverage is 88.37%.

@@            Coverage Diff             @@
##           master    #4465      +/-   ##
==========================================
+ Coverage   81.19%   81.25%   +0.06%     
==========================================
  Files         281      282       +1     
  Lines        7981     8003      +22     
==========================================
+ Hits         6480     6503      +23     
  Misses       1112     1112              
+ Partials      389      388       -1

Impacted Files	Coverage Δ
...econciler/inmemorychannel/dispatcher/controller.go	`78.26% <20.00%> (ø)`
pkg/kncloudevents/message_sender.go	`78.00% <50.00%> (-1.67%)`	⬇️
pkg/adapter/v2/main_message.go	`32.50% <100.00%> (ø)`
pkg/channel/message_dispatcher.go	`77.31% <100.00%> (-0.24%)`	⬇️
pkg/kncloudevents/http_client.go	`100.00% <100.00%> (ø)`
pkg/mtbroker/filter/filter_handler.go	`78.78% <100.00%> (+0.26%)`	⬆️
...iler/inmemorychannel/dispatcher/inmemorychannel.go	`89.39% <100.00%> (+0.86%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44e2412...695cf4a. Read the comment docs.

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

pkg/kncloudevents/http_client.go

pkg/mtbroker/ingress/ingress_handler_test.go

pkg/reconciler/inmemorychannel/dispatcher/inmemorychannel.go

pkg/reconciler/inmemorychannel/dispatcher/testdata/config-event-dispatcher.yaml

pkg/kncloudevents/http_client.go

vaikas · 2020-11-05T11:31:58Z

pkg/kncloudevents/message_sender.go


-	return &HTTPMessageSender{Client: client, Target: target}, nil
+func NewHTTPMessageSenderWithTarget(target string) (*HTTPMessageSender, error) {
+	return &HTTPMessageSender{Client: getClient(), Target: target}, nil


Just an observation/thought, probably silly, but if we pass in here interface to how to get the client, we don't need the global resync at the cost of the doing the Read lock on the mutex guarding the client. This might make it easier to write tests also? Curious on what the overhead for a read mutex would be.

I completely disagree with this idea 😄 Putting a lock, shared between all dispatchers in the instance, on the hot path, doesn't sound at all like a good idea IMO.

To be honest, I think the configuration of these http params shouldn't be allowed at all during the execution of the dispatcher. It should be a single read when the process is started and that's it. These are params that are usually configured only at setup time, when you "setup" the capacity of your deployments: you don't change it on fly.

If you do, then you expect in any case a sort of "downtime"... So what I did in this PR is that I've done the bare minimum to fix the issue, I would love to open later a discussions on these params and why they should be configurable only at startup (and not like watches)

Yeah, just jotted it down :) I figured you'd be a fan :)
I'm curious also on how often these in practice need to change.

I guess never, hence my idea of removing that watch

If that complexity is not needed in the first place, removing it in another PR sounds like an excellent idea to me. Does anyone know if there is/was a use-case in Eventing for reconfiguring the transport on the fly?

pkg/reconciler/inmemorychannel/dispatcher/inmemorychannel.go

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

antoineco · 2020-11-05T11:54:57Z

/lgtm

slinkydeveloper · 2020-11-05T12:05:02Z

/hold for another pass

pkg/kncloudevents/http_client.go

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-metrics-robot · 2020-11-05T13:48:38Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/channel/message_dispatcher.go	86.4%	86.2%	-0.2
pkg/kncloudevents/http_client.go	Do not exist	100.0%
pkg/kncloudevents/message_sender.go	82.9%	82.9%	-0.1
pkg/reconciler/inmemorychannel/dispatcher/controller.go	84.8%	79.4%	-5.4

matzew

/lgtm

knative-prow-robot · 2020-11-05T14:01:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, slinkydeveloper

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~~ [matzew,slinkydeveloper]

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

slinkydeveloper · 2020-11-05T14:31:24Z

/unhold

slinkydeveloper · 2020-11-05T14:47:58Z

Backporting to 0.17 and 0.18

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper · 2020-11-05T15:06:57Z

Backport 0.17 #4467 and 0.18 #4468

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> Co-authored-by: Francesco Guardiani <francescoguard@gmail.com>

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

* Update pingsource-mt-adapter.yaml * Like on 0.18.3, we skip the tracing tests Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * [release-0.18] Retry on network failures (knative#4454) (knative#4457) * Retry on network failures (knative#4454) Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * nethttp -> http Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Backport knative#4465 (knative#4468) Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * [0.18] Backport knative#4466 (knative#4471) * Remove double invocations to responseWriter.WriteHeader in filter handler (knative#4466) * Fix knative#4464 Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Docs Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Moar tests Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Linting Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Nit with metrics Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> (cherry picked from commit a6fc540) Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Nit Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * fixed wrong marshall in apiserversouece which will fix the missing ceOverrides extension (knative#4477) (knative#4480) * fixed wrong marshall * fixed UT * [0.18] Readyness probe in broker ingress (knative#4483) * Fix knative#4473 Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Massage the filter yaml Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> Co-authored-by: Francesco Guardiani <francescoguard@gmail.com> Co-authored-by: capri-xiyue <52932582+capri-xiyue@users.noreply.github.com>

Fix knative#4461

a972c70

Now every new message sender always reuse the same underlying client, whenever possible Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 5, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 5, 2020

knative-prow-robot requested review from grantr and n3wscott November 5, 2020 08:55

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020

slinkydeveloper added 4 commits November 5, 2020 10:13

Increase coverage

563e857

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Brought back the previous method to avoid breakage

dede5c0

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Now we use nice language

5df0078

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Removed useless test

7ac150f

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

antoineco reviewed Nov 5, 2020

View reviewed changes

vaikas reviewed Nov 5, 2020

View reviewed changes

slinkydeveloper added 3 commits November 5, 2020 12:36

Suggestions

1c1897a

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Imports job

7b9bff4

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

nit

29f546b

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-prow-robot assigned antoineco Nov 5, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2020

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2020

vaikas reviewed Nov 5, 2020

View reviewed changes

pkg/kncloudevents/http_client.go Show resolved Hide resolved

Fancy ut

7a87ff5

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2020

Copyright

695cf4a

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

matzew approved these changes Nov 5, 2020

View reviewed changes

knative-prow-robot assigned matzew Nov 5, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2020

knative-prow-robot merged commit 3e5eeab into knative:master Nov 5, 2020

slinkydeveloper deleted the issues/4461 branch November 5, 2020 14:33

slinkydeveloper added a commit to slinkydeveloper/eventing that referenced this pull request Nov 5, 2020

Backport knative#4465

e739e10

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper mentioned this pull request Nov 5, 2020

[0.17] Backport #4465 #4467

Merged

slinkydeveloper added a commit to slinkydeveloper/eventing that referenced this pull request Nov 5, 2020

Backport knative#4465

87de928

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper mentioned this pull request Nov 5, 2020

[0.18] Backport #4465 #4468

Merged

knative-prow-robot pushed a commit that referenced this pull request Nov 5, 2020

Backport #4465 (#4467)

2fb113c

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

matzew pushed a commit to matzew/eventing that referenced this pull request Nov 5, 2020

Backport knative#4465 (knative#4467)

7147011

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper mentioned this pull request Nov 5, 2020

Remove watch on config-imc-event-dispatcher #4469

Closed

knative-prow-robot pushed a commit that referenced this pull request Nov 5, 2020

Backport #4465 (#4468)

650096a

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

openshift-merge-robot pushed a commit to openshift/knative-eventing that referenced this pull request Nov 5, 2020

Backport knative#4465 (knative#4467) (#951)

2db3055

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> Co-authored-by: Francesco Guardiani <francescoguard@gmail.com>

matzew mentioned this pull request Nov 6, 2020

No longer use deprecated calls knative-extensions/eventing-kafka#169

Merged

matzew pushed a commit to matzew/eventing that referenced this pull request Nov 7, 2020

Backport knative#4465 (knative#4468)

7c4225e

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

matzew pushed a commit to openshift/knative-eventing that referenced this pull request Nov 7, 2020

Backport knative#4465 (knative#4468)

1e327dc

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper mentioned this pull request Feb 1, 2021

kafka-ch-dispatcher doesn't reuse outgoing connections knative-extensions/eventing-kafka#342

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kncloudevents now uses a singleton http client #4465

kncloudevents now uses a singleton http client #4465

slinkydeveloper commented Nov 5, 2020

slinkydeveloper commented Nov 5, 2020

codecov bot commented Nov 5, 2020 •

edited

Loading

vaikas Nov 5, 2020

slinkydeveloper Nov 5, 2020 •

edited

Loading

vaikas Nov 5, 2020

slinkydeveloper Nov 5, 2020

antoineco Nov 5, 2020

slinkydeveloper Nov 5, 2020

antoineco commented Nov 5, 2020

slinkydeveloper commented Nov 5, 2020

knative-metrics-robot commented Nov 5, 2020

matzew left a comment

knative-prow-robot commented Nov 5, 2020

slinkydeveloper commented Nov 5, 2020

slinkydeveloper commented Nov 5, 2020

slinkydeveloper commented Nov 5, 2020

kncloudevents now uses a singleton http client #4465

kncloudevents now uses a singleton http client #4465

Conversation

slinkydeveloper commented Nov 5, 2020

Proposed Changes

slinkydeveloper commented Nov 5, 2020

codecov bot commented Nov 5, 2020 • edited Loading

Codecov Report

vaikas Nov 5, 2020

Choose a reason for hiding this comment

slinkydeveloper Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

vaikas Nov 5, 2020

Choose a reason for hiding this comment

slinkydeveloper Nov 5, 2020

Choose a reason for hiding this comment

antoineco Nov 5, 2020

Choose a reason for hiding this comment

slinkydeveloper Nov 5, 2020

Choose a reason for hiding this comment

antoineco commented Nov 5, 2020

slinkydeveloper commented Nov 5, 2020

knative-metrics-robot commented Nov 5, 2020

matzew left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 5, 2020

slinkydeveloper commented Nov 5, 2020

slinkydeveloper commented Nov 5, 2020

slinkydeveloper commented Nov 5, 2020

codecov bot commented Nov 5, 2020 •

edited

Loading

slinkydeveloper Nov 5, 2020 •

edited

Loading