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

Non-blocking data plane components #438

Conversation

pierDipi
Copy link
Member

Sharing a WebClient across multiple verticles is discouraged by
Vertx, see https://vertx.io/docs/vertx-core/java/#_httpclient_usage.
That section talks about the HTTP Client, the WebClient isn't different.

When used in a Verticle, the Verticle should use its own client instance.

Main changes are around when objects are built and in which Vertx context.

Proposed Changes

  • Dispatcher non-blocking
  • Receiver non-blocking

Release Note

- :broom: Update or clean up current behavior
The data plane is completely non-blocking.

/kind enhancement

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 27, 2020
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 27, 2020
@codecov
Copy link

codecov bot commented Nov 27, 2020

Codecov Report

Merging #438 (23b8288) into master (e83bd49) will decrease coverage by 0.21%.
The diff coverage is 84.48%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #438      +/-   ##
============================================
- Coverage     77.35%   77.14%   -0.22%     
  Complexity      262      262              
============================================
  Files            59       59              
  Lines          1877     1877              
  Branches         83       83              
============================================
- Hits           1452     1448       -4     
- Misses          320      321       +1     
- Partials        105      108       +3     
Flag Coverage Δ Complexity Δ
java-unittests 77.12% <84.48%> (-0.36%) 0.00 <13.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...knative/eventing/kafka/broker/dispatcher/Main.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...v/knative/eventing/kafka/broker/receiver/Main.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...enting/kafka/broker/receiver/ReceiverVerticle.java 93.10% <80.00%> (+0.24%) 3.00 <2.00> (ø)
...r/dispatcher/http/HttpConsumerVerticleFactory.java 88.52% <87.17%> (-6.64%) 22.00 <9.00> (ø)
...ting/kafka/broker/dispatcher/ConsumerVerticle.java 100.00% <100.00%> (ø) 3.00 <2.00> (ø)
...tive/eventing/kafka/broker/core/filter/Filter.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)

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 e83bd49...5a65fa0. Read the comment docs.

@knative-prow-robot 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 27, 2020
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
@pierDipi pierDipi force-pushed the create-web-client-per-verticle branch from 0e1cee1 to ba5f9da Compare November 27, 2020 16:20
@pierDipi
Copy link
Member Author

/unhold

@knative-prow-robot 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 27, 2020
@pierDipi
Copy link
Member Author

pierDipi commented Dec 1, 2020

@slinkydeveloper can we merge this?

@slinkydeveloper
Copy link
Contributor

I wanted to do another pass, i'm on it

Copy link
Contributor

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

I find the code here a little bit convoluted, I left some minor comments and we can go ahead merging it. I would love to improve it in next iterations to simplify it

@pierDipi pierDipi force-pushed the create-web-client-per-verticle branch 2 times, most recently from d9bd758 to bd65169 Compare December 1, 2020 11:05
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
@pierDipi pierDipi force-pushed the create-web-client-per-verticle branch from bd65169 to 5a65fa0 Compare December 1, 2020 11:11
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, 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 [pierDipi,slinkydeveloper]

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

@pierDipi
Copy link
Member Author

pierDipi commented Dec 1, 2020

Can you lgtm?

@slinkydeveloper
Copy link
Contributor

/lgtm

Sorry i forgot it 😄

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020
@pierDipi
Copy link
Member Author

pierDipi commented Dec 1, 2020

No problem :)

@knative-prow-robot knative-prow-robot merged commit 50e92f9 into knative-extensions:master Dec 1, 2020
pierDipi added a commit to pierDipi/eventing-kafka-broker that referenced this pull request Feb 27, 2021
* Dispatcher non-blocking

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

* Receiver non-blocking

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

* Refactor our core factory and rename factories

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

* Rename method, delete comments and use HashMap constructor

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
@pierDipi pierDipi deleted the create-web-client-per-verticle branch April 14, 2021 21:16
matzew pushed a commit to matzew/eventing-kafka-broker that referenced this pull request Nov 4, 2022
…ensions#2720) (knative-extensions#438)

We're setting it after a Consumer is created.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. kind/enhancement lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants