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

Broker ingress and Dispatcher now uses the protocol binding instead of json to process events #751

Conversation

gabo1208
Copy link
Contributor

@gabo1208 gabo1208 commented May 9, 2022

Merge after #750

Changes

  • 🎁 Now the Broker's ingress uses the Binary representation of the CloudEvents
  • 🎁 Now the Broker's Dispatcher uses the protocol binding to get a CloudEvent from a RabbitMQ Message
  • 🎁 Added performance tests for the source using the new Ingress CE Binary represenation, that makes the Ingress plugable to the Source
  • 🧹 Updated Broker's performance tests results to reflect the performance gains (like 10% less latency and less cpu and memory consumption overall)

/kind enhancement performance

Fixes #

Release Note

- 🎁 Now the Broker's ingress uses the Binary representation of the CloudEvents
- 🎁 Now the Broker's Dispatcher uses the protocol binding to get a CloudEvent from a RabbitMQ Message
- 🎁 Added performance tests for the source using the new Ingress CE Binary represenation, that makes the Ingress plugable to the Source

@knative-prow knative-prow bot added kind/enhancement kind/performance size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 9, 2022
@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #751 (ea9a3bf) into main (291010d) will decrease coverage by 0.36%.
The diff coverage is 55.38%.

@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
- Coverage   75.20%   74.84%   -0.37%     
==========================================
  Files          36       35       -1     
  Lines        2654     2544     -110     
==========================================
- Hits         1996     1904      -92     
+ Misses        588      576      -12     
+ Partials       70       64       -6     
Impacted Files Coverage Δ
pkg/rabbit/message.go 68.90% <53.06%> (ø)
pkg/adapter/adapter.go 60.84% <62.50%> (+2.34%) ⬆️
pkg/dispatcher/dispatcher.go

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 291010d...ea9a3bf. Read the comment docs.

@gabo1208 gabo1208 force-pushed the protocol-spec-for-broker-and-source branch 3 times, most recently from b27ea9c to c8a46a9 Compare May 10, 2022 03:43
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2022
…ntry point for the source in the conformance tests, the changes to the broker to support this are going to be made in another commit + created source perf test results and published images, the performance gains while using the binary protocol binding vs json manipulation seems to be significant memorywise
using ce protocol binding to get msg from a rabbitmq msg delivery
… does not support header on its test channels

excluding adapter from the golangci-lint while wabbit is been removed from the repo

added missing ce attributes to msg headers in ingres

added source to message header so it will be filtered appropriately

added support for timestamp in the ingress formating

trying different approach with cloudevents and filters
@gabo1208 gabo1208 force-pushed the protocol-spec-for-broker-and-source branch from adb2b61 to 779cd76 Compare May 10, 2022 17:14
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2022
fixed broker perf test setup + removed unused file + rebased
removed ununsed header

using the contentype header

avoid using rabbitmq specific headers when translating rabbitmq messages to cloudevents + small refactor on the dispatcher
… rabbitmq message + fixed malformed json in e2e test producer

fixing e2e and conformance tests to match expected output

removed unnecesary cast
@gabo1208 gabo1208 force-pushed the protocol-spec-for-broker-and-source branch from 2a82180 to c839f5a Compare May 11, 2022 17:31
Copy link
Contributor

@gab-satchi gab-satchi left a comment

Choose a reason for hiding this comment

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

thanks for the changes. just some minor things

pkg/adapter/adapter_test.go Outdated Show resolved Hide resolved
pkg/dispatcher/dispatcher_test.go Show resolved Hide resolved
@gab-satchi
Copy link
Contributor

/retest

Copy link
Contributor

@gab-satchi gab-satchi left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2022
@knative-prow
Copy link

knative-prow bot commented May 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gab-satchi, gabo1208

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 [gab-satchi,gabo1208]

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

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. kind/enhancement kind/performance lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants