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.

Sign up for GitHub

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

Jump to bottom

Add Receiver loom based module Implementation #3215

Merged
merged 36 commits into from
Aug 1, 2023
Merged

Add Receiver loom based module Implementation #3215

merged 36 commits into from
Aug 1, 2023

Conversation

debasishbsws
Copy link
Member

Progress in #2928

Proposed Changes

  • Loom-based module implementation
  • KafkaProducer unit Test
  • modify the tests module DataPlaneTest to be runnable for both vertx and loom modules

Release Note

Docs

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2023
@knative-prow
Copy link

knative-prow bot commented Jul 15, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/data-plane labels Jul 15, 2023
@codecov
Copy link

codecov bot commented Jul 17, 2023 •
edited
Loading

Codecov Report

Merging #3215 (eab64fa) into main (6794484) will decrease coverage by 1.60%.
Report is 12 commits behind head on main.
The diff coverage is 6.47%.

@@             Coverage Diff              @@
##               main    #3215      +/-   ##
============================================
- Coverage     63.61%   62.02%   -1.60%     
+ Complexity      775      735      -40     
============================================
  Files           170      176       +6     
  Lines         11948    12085     +137     
  Branches        250      253       +3     
============================================
- Hits           7601     7496     -105     
- Misses         3770     4001     +231     
- Partials        577      588      +11
Flag Coverage Δ
java-unittests 72.97% <0.00%> (-7.35%) ⬇️

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

Files Changed Coverage Δ
control-plane/pkg/contract/contract.pb.go 7.41% <0.00%> (-0.04%) ⬇️
...afka/broker/core/tracing/kafka/ProducerTracer.java 0.00% <0.00%> (ø)
.../kafka/broker/core/tracing/kafka/TraceContext.java 0.00% <0.00%> (ø)
...ing/kafka/broker/core/tracing/kafka/TraceTags.java 0.00% <0.00%> (ø)
...g/kafka/broker/receiverloom/LoomKafkaProducer.java 0.00% <0.00%> (ø)
...kafka/broker/receiverloom/LoomProducerFactory.java 0.00% <0.00%> (ø)
...ative/eventing/kafka/broker/receiverloom/Main.java 0.00% <0.00%> (ø)
control-plane/pkg/reconciler/broker/broker.go 72.50% <100.00%> (+0.63%) ⬆️
control-plane/pkg/reconciler/channel/channel.go 69.10% <100.00%> (+0.05%) ⬆️
...ntrol-plane/pkg/reconciler/channel/v2/channelv2.go 70.96% <100.00%> (+0.04%) ⬆️
... and 1 more

... and 15 files with indirect coverage changes

@debasishbsws
Copy link
Member Author

Need to add the tracing and span for the producer

@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 18, 2023
@knative-prow-robot knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 20, 2023
@debasishbsws
Copy link
Member Author

/test all

@debasishbsws
Copy link
Member Author

I have found the problem with DataPlaneTest I was having and fixed it.
Now I am trying to refactor the DataPlaneTest and trying to make it abstract for both Implementations.

Is it okay to make the methods non-static and use @BeforeEach and @AfterEach? I haven't found any reason to make them static. But just wanted to confirm.
/cc @pierDipi

@knative-prow knative-prow bot requested a review from pierDipi July 26, 2023 14:11
@pierDipi
Copy link
Member

I have found the problem with DataPlaneTest I was having and fixed it. Now I am trying to refactor the DataPlaneTest and trying to make it abstract for both Implementations.

Great job @debasishbsws !

Is it okay to make the methods non-static and use @BeforeEach and @AfterEach? I haven't found any reason to make them static. But just wanted to confirm. /cc @pierDipi

Yes, I don't see why not!

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2023
debasishbsws added 2 commits July 31, 2023 18:44
Make wait 10 sec before check thread
6c24bee 
Signed-off-by: debasishbsws <debasishbsws.abc@gmail.com>
Some refactore
6c77c83 
Signed-off-by: debasishbsws <debasishbsws.abc@gmail.com>
Test fixed
ac6792d 
Signed-off-by: debasishbsws <debasishbsws.abc@gmail.com>
debasishbsws added 3 commits July 31, 2023 21:51
Complete the promise
27e95fd 
Signed-off-by: debasishbsws <debasishbsws.abc@gmail.com>
fix the test mock to implemente send with callback
221b512 
Signed-off-by: debasishbsws <debasishbsws.abc@gmail.com>
wait to join and end of thread
8192ddb 
Signed-off-by: debasishbsws <debasishbsws.abc@gmail.com>
pierDipi
pierDipi reviewed Aug 1, 2023
View reviewed changes
...ver-loom/src/main/java/dev/knative/eventing/kafka/broker/receiverloom/LoomKafkaProducer.java Outdated Show resolved Hide resolved
...loom/src/test/java/dev/knative/eventing/kafka/broker/receiverloom/LoomKafkaProducerTest.java Outdated Show resolved Hide resolved
...loom/src/test/java/dev/knative/eventing/kafka/broker/receiverloom/LoomKafkaProducerTest.java Outdated Show resolved Hide resolved
...loom/src/test/java/dev/knative/eventing/kafka/broker/receiverloom/LoomKafkaProducerTest.java Outdated Show resolved Hide resolved
...loom/src/test/java/dev/knative/eventing/kafka/broker/receiverloom/LoomKafkaProducerTest.java Outdated Show resolved Hide resolved
...loom/src/test/java/dev/knative/eventing/kafka/broker/receiverloom/LoomKafkaProducerTest.java Outdated Show resolved Hide resolved
...loom/src/test/java/dev/knative/eventing/kafka/broker/receiverloom/LoomKafkaProducerTest.java Outdated Show resolved Hide resolved
...loom/src/test/java/dev/knative/eventing/kafka/broker/receiverloom/LoomKafkaProducerTest.java Outdated Show resolved Hide resolved
...loom/src/test/java/dev/knative/eventing/kafka/broker/receiverloom/LoomKafkaProducerTest.java Outdated Show resolved Hide resolved
debasishbsws added 2 commits August 1, 2023 17:46
Suggested cgenges in test and close to complete
2be5c02 
Signed-off-by: debasishbsws <debasishbsws.abc@gmail.com>
change sendFromQueue
dbb47e3 
Signed-off-by: debasishbsws <debasishbsws.abc@gmail.com>
debasishbsws added 2 commits August 1, 2023 18:25
change code seq and some comments
4aa9d86 
Signed-off-by: debasishbsws <debasishbsws.abc@gmail.com>
set the numRecord final
eab64fa 
Signed-off-by: debasishbsws <debasishbsws.abc@gmail.com>
matzew
matzew approved these changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

/hold
(for others to review - or just unhold)

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2023
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2023
@knative-prow
Copy link

knative-prow bot commented Aug 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2023
@pierDipi
Copy link
Member

pierDipi commented Aug 1, 2023

/unhold
/lgtm

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2023
@knative-prow knative-prow bot merged commit 76e992e into knative-extensions:main Aug 1, 2023
14 of 18 checks passed
@pierDipi pierDipi changed the title adding Receiver loom based module Implementation. Add Receiver loom based module Implementation Aug 1, 2023
@debasishbsws debasishbsws mentioned this pull request Aug 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi left review comments

@matzew matzew matzew approved these changes

@aliok aliok Awaiting requested review from aliok

@lionelvillard lionelvillard Awaiting requested review from lionelvillard

Assignees

@matzew matzew

@pierDipi pierDipi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/data-plane lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@debasishbsws @pierDipi @matzew @knative-prow-robot