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

Migrate current dispatcher to use ReactiveKafkaConsumer instead of Vertx KafkaConsumer. #3177

Merged
merged 19 commits into from
Jul 4, 2023

Conversation

debasishbsws
Copy link
Member

@debasishbsws debasishbsws commented Jun 27, 2023

progress #2928
The new dispatcher-vertx module is the one that uses Vertx implementation.

Proposed Changes

  • Remove all the Vertx Kafka client dependencies from the base dispatcher module.
  • Change the current main() to start(..) and call from the other module with specialised Consumer implementation as args.
  • migrate all the tests from Direct KafkaConsumer to use ReactiveKafkaConsumer Interface.

Aditional Info

  • The implementation of passing handler function to ConsumerRebalanceListener has gotten complex. I will simplify that further in the future.
  • Tests for the dispatcher-vertx module are not there as vertx implementations are already well tested.

@knative-prow
Copy link

knative-prow bot commented Jun 27, 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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/data-plane labels Jun 27, 2023
@knative-prow knative-prow bot requested review from aliok and pierDipi June 27, 2023 19:18
@knative-prow knative-prow bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 27, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #3177 (ebff383) into main (fb89db6) will decrease coverage by 0.04%.
The diff coverage is 84.37%.

@@             Coverage Diff              @@
##               main    #3177      +/-   ##
============================================
- Coverage     63.44%   63.41%   -0.04%     
+ Complexity      757      753       -4     
============================================
  Files           167      167              
  Lines         11756    11791      +35     
  Branches        239      238       -1     
============================================
+ Hits           7459     7477      +18     
- Misses         3736     3753      +17     
  Partials        561      561              
Flag Coverage Δ
java-unittests 79.82% <84.37%> (-0.34%) ⬇️

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

Impacted Files Coverage Δ
...enting/kafka/broker/core/OrderedAsyncExecutor.java 58.92% <0.00%> (ø)
...ker/dispatcher/impl/consumer/ConsumerVerticle.java 87.09% <ø> (-6.46%) ⬇️
...broker/dispatcher/impl/consumer/OffsetManager.java 95.78% <ø> (ø)
...oker/dispatcher/main/ConsumerDeployerVerticle.java 67.79% <ø> (ø)
...ve/eventing/kafka/broker/dispatcher/main/Main.java 0.00% <0.00%> (ø)
...patcher/impl/consumer/OrderedConsumerVerticle.java 65.38% <60.00%> (+0.33%) ⬆️
...roker/dispatcher/main/ConsumerVerticleBuilder.java 66.33% <82.60%> (-2.74%) ⬇️
...a/broker/dispatcher/impl/RecordDispatcherImpl.java 89.77% <93.75%> (-0.05%) ⬇️
.../broker/dispatcher/impl/ConsumerRecordContext.java 100.00% <100.00%> (ø)
.../dispatcher/impl/RecordDispatcherMutatorChain.java 100.00% <100.00%> (ø)
... and 3 more

... and 4 files with indirect coverage changes

@debasishbsws
Copy link
Member Author

@pierDipi In the last commit basically I create ConsumerRebalanceListener from the PartisionsRevokeHandlers and pass it to ConsumerVirticleContext and then while calling the subscribe() method I am passing that ConsumerRebalanceListener with it. There may by some redundancy now like in future I can change the PartisionsRevokeHandlers in order to reduce the complexity.

@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 30, 2023
@debasishbsws
Copy link
Member Author

Didn't look into the Benchmark module properly just migrate it to use Vertx implementation.

@debasishbsws
Copy link
Member Author

/test all

@debasishbsws
Copy link
Member Author

/retest-required

@debasishbsws
Copy link
Member Author

debasishbsws commented Jul 2, 2023

I am not sure why the profiler test is failing. maybe because of codegen for now.

@debasishbsws
Copy link
Member Author

debasishbsws commented Jul 2, 2023

Tests are saying to run update-codegen.sh, but after running that DataPlaneContract.java's hasReplyUrf() method is being removed.
image

But we need that method as we call it from
https://github.com/knative-sandbox/eventing-kafka-broker/blob/fb89db60df3fdb7d6ae6721037a081b5d88a60c8/data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/main/ConsumerVerticleBuilder.java#L204-L206

@debasishbsws debasishbsws marked this pull request as ready for review July 2, 2023 12:18
@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 Jul 2, 2023
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.

/lgtm
/approve

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

knative-prow bot commented Jul 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Jul 4, 2023
@knative-prow knative-prow bot merged commit 4dd4f24 into knative-extensions:main Jul 4, 2023
31 of 32 checks passed
@debasishbsws debasishbsws deleted the migrate-dispatcher branch July 7, 2023 04:05
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. area/data-plane 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

2 participants