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 Dispatcher loom based module Implementation #3257

Merged
merged 11 commits into from
Aug 14, 2023
Merged

Add Dispatcher loom based module Implementation #3257

merged 11 commits into from
Aug 14, 2023

Conversation

debasishbsws
Copy link
Member

Progress in #2928

Proposed Changes

  • Loom-based dispatcher module implementation
  • Unit Test for them

@knative-prow
Copy link

knative-prow bot commented Aug 1, 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 the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2023
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/data-plane labels Aug 1, 2023
@debasishbsws
Copy link
Member Author

/cc @pierDipi

For early review on this POC.

@knative-prow knative-prow bot requested a review from pierDipi August 1, 2023 17:01
@codecov
Copy link

codecov bot commented Aug 1, 2023 •
edited
Loading

Codecov Report

Merging #3257 (073de91) into main (76e992e) will decrease coverage by 0.26%.
Report is 14 commits behind head on main.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #3257      +/-   ##
============================================
- Coverage     62.05%   61.79%   -0.26%     
- Complexity      736      765      +29     
============================================
  Files           176      181       +5     
  Lines         12085    12219     +134     
  Branches        253      266      +13     
============================================
+ Hits           7499     7551      +52     
- Misses         3999     4080      +81     
- Partials        587      588       +1
Flag Coverage Δ
java-unittests 71.57% <0.00%> (-1.51%) ⬇️

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

Files Changed Coverage Δ
...fka/broker/dispatcherloom/LoomConsumerFactory.java 0.00% <0.00%> (ø)
...kafka/broker/dispatcherloom/LoomKafkaConsumer.java 0.00% <0.00%> (ø)
...ive/eventing/kafka/broker/dispatcherloom/Main.java 0.00% <0.00%> (ø)

... and 9 files with indirect coverage changes

pierDipi
pierDipi reviewed Aug 2, 2023
View reviewed changes
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

This looks good overall!

I left a few minor comments and the requested help for the threading model on close.

...r-loom/src/main/java/dev/knative/eventing/kafka/broker/dispatcherloom/LoomKafkaConsumer.java Outdated Show resolved Hide resolved
...r-loom/src/main/java/dev/knative/eventing/kafka/broker/dispatcherloom/LoomKafkaConsumer.java Outdated Show resolved Hide resolved
...r-loom/src/main/java/dev/knative/eventing/kafka/broker/dispatcherloom/LoomKafkaConsumer.java Outdated Show resolved Hide resolved
...r-loom/src/main/java/dev/knative/eventing/kafka/broker/dispatcherloom/LoomKafkaConsumer.java Outdated Show resolved Hide resolved
...r-loom/src/main/java/dev/knative/eventing/kafka/broker/dispatcherloom/LoomKafkaConsumer.java Outdated Show resolved Hide resolved
...r-loom/src/main/java/dev/knative/eventing/kafka/broker/dispatcherloom/LoomKafkaConsumer.java Outdated Show resolved Hide resolved
...r-loom/src/main/java/dev/knative/eventing/kafka/broker/dispatcherloom/LoomKafkaConsumer.java Outdated Show resolved Hide resolved
...r-loom/src/main/java/dev/knative/eventing/kafka/broker/dispatcherloom/LoomKafkaConsumer.java Outdated Show resolved Hide resolved
@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 Aug 3, 2023
@debasishbsws debasishbsws marked this pull request as ready for review August 3, 2023 14:47
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 3, 2023
@knative-prow knative-prow bot requested a review from aliok August 3, 2023 14:47
@debasishbsws
Copy link
Member Author

/retest

pierDipi
pierDipi reviewed Aug 3, 2023
View reviewed changes
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

Great job @debasishbsws!

...om/src/test/java/dev/knative/eventing/kafka/broker/dispatcherloom/LoomKafkaConsumerTest.java Outdated Show resolved Hide resolved
...om/src/test/java/dev/knative/eventing/kafka/broker/dispatcherloom/LoomKafkaConsumerTest.java Outdated Show resolved Hide resolved
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 8, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@debasishbsws
more Unit test
5218d80 
Signed-off-by: debasishbsws <[email protected]>
@debasishbsws
enable priview feature and fix test
57d6c03 
Signed-off-by: debasishbsws <[email protected]>
@debasishbsws
name change
8f734d8 
Signed-off-by: debasishbsws <[email protected]>
@debasishbsws
Suggested changes
366439c 
Signed-off-by: debasishbsws <[email protected]>
@debasishbsws
Add comment on --enable-priview
073de91 
Signed-off-by: debasishbsws <[email protected]>
@debasishbsws
Copy link
Member Author

Hey @matzew, I have added the comment.

@debasishbsws
Copy link
Member Author

/retest-required

matzew
matzew approved these changes Aug 14, 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

nice work (I checked this a bit already last week, and pierdipi was also already reviewing parts here)

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2023
@knative-prow
Copy link

knative-prow bot commented Aug 14, 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 14, 2023
@knative-prow knative-prow bot merged commit 842eb1c into knative-extensions:main Aug 14, 2023
15 of 18 checks passed
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

@odacremolbap odacremolbap Awaiting requested review from odacremolbap

@aliok aliok Awaiting requested review from aliok

Assignees

@matzew matzew

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.
3 participants
@debasishbsws @matzew @pierDipi