Dispatcher verticles are now worker verticles #2300

pierDipi · 2022-06-13T15:05:06Z

Using worker verticles instead of event loop verticle
provides better isolation between consumers.

I added a reproducer for #2263 (thanks Marek)
that is now running in CI during KafkaChannel testing.

I've also improved logging which helped me tracking
down the issue.

As Vert.x's docs says:

Worker verticle instances are never executed
concurrently by Vert.x by more than one thread,
but can executed by different threads at different times.

That's the reason why we're moving to synchronized
and atomic data structures for dispatcher components.

Fixes #2263

pierDipi · 2022-06-13T17:11:14Z

/test channel-integration-tests-sasl-plain_eventing-kafka-broker_main

pierDipi · 2022-06-13T17:14:43Z

/test channel-integration-tests-sasl-plain_eventing-kafka-broker_main

pierDipi · 2022-06-13T17:22:18Z

/test channel-integration-tests-sasl-plain_eventing-kafka-broker_main

pierDipi · 2022-06-14T06:38:09Z

/test channel-integration-tests-sasl-plain_eventing-kafka-broker_main
/test channel-integration-tests-ssl_eventing-kafka-broker_main
/test channel-integration-tests-sasl-ssl_eventing-kafka-broker_main

pierDipi · 2022-06-14T08:15:59Z

/test channel-integration-tests-sasl-plain_eventing-kafka-broker_main

pierDipi · 2022-06-14T08:28:09Z

Worked, retesting:

/test channel-integration-tests-sasl-ssl_eventing-kafka-broker_main
/test channel-integration-tests-ssl_eventing-kafka-broker_main

pierDipi · 2022-06-14T09:27:10Z

unrelated EOF error knative/pkg#1509:

Error from server (InternalError): error when creating "STDIN": Internal error occurred: failed calling webhook "sinkbindings.webhook.sources.knative.dev": failed to call webhook: Post "[https://eventing-webhook.knative-eventing.svc:443/sinkbindings?timeout=10s](https://eventing-webhook.knative-eventing.svc/sinkbindings?timeout=10s)": EOF

and timeout on KafkaChannel readiness:

kafkachannel.messaging.knative.dev/channel created
subscription.messaging.knative.dev/event-display created
service/event-display created
deployment.apps/event-display created
sinkbinding.sources.knative.dev/bind-heartbeat created
deployment.apps/hearbeat created
error: timed out waiting for the condition on kafkachannels/channel

pierDipi · 2022-06-14T09:27:33Z

Retesting the EOF error:
/test channel-integration-tests-sasl-ssl_eventing-kafka-broker_main

pierDipi · 2022-06-14T10:58:56Z

There are no nodes that your pod can schedule to - check your requests, tolerations, and node selectors (skip schedule deleting pod: test-pods/e7440397-ebcc-11ec-a2fb-666d91f9e04d)

pierDipi · 2022-06-14T10:59:06Z

/retest-required

codecov · 2022-06-14T11:05:15Z

Codecov Report

Merging #2300 (f61381e) into main (ebb10be) will decrease coverage by 0.07%.
The diff coverage is 71.79%.

@@             Coverage Diff              @@
##               main    #2300      +/-   ##
============================================
- Coverage     65.51%   65.44%   -0.08%     
  Complexity      699      699              
============================================
  Files           140      140              
  Lines          9216     9240      +24     
  Branches        209      211       +2     
============================================
+ Hits           6038     6047       +9     
- Misses         2777     2789      +12     
- Partials        401      404       +3

Flag	Coverage Δ
java-unittests	`81.64% <71.79%> (-0.57%)`	⬇️

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

Impacted Files	Coverage Δ
...e/eventing/kafka/broker/core/file/FileWatcher.java	`74.68% <0.00%> (-0.96%)`	⬇️
...ka/broker/core/reconciler/ResourcesReconciler.java	`100.00% <ø> (ø)`
...patcher/impl/consumer/OrderedConsumerVerticle.java	`70.52% <55.17%> (-4.48%)`	⬇️
...ispatcher/impl/http/WebClientCloudEventSender.java	`60.43% <57.14%> (-0.03%)`	⬇️
...ve/eventing/kafka/broker/core/metrics/Metrics.java	`84.61% <66.66%> (-1.60%)`	⬇️
...tcher/impl/consumer/UnorderedConsumerVerticle.java	`77.55% <70.00%> (-2.05%)`	⬇️
...a/broker/dispatcher/impl/RecordDispatcherImpl.java	`87.91% <77.77%> (-0.49%)`	⬇️
...dispatcher/impl/consumer/BaseConsumerVerticle.java	`87.09% <83.33%> (-6.66%)`	⬇️
...ive/eventing/kafka/broker/core/AsyncCloseable.java	`76.92% <100.00%> (-14.75%)`	⬇️
.../core/reconciler/impl/ResourcesReconcilerImpl.java	`90.66% <100.00%> (+0.38%)`	⬆️
... and 11 more

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 ebb10be...f61381e. Read the comment docs.

pierDipi · 2022-06-14T14:05:08Z

Job timed out after 2h but it was working

{"component":"entrypoint","file":"k8s.io/test-infra/prow/entrypoint/run.go:255","func":"k8s.io/test-infra/prow/entrypoint.gracefullyTerminate","level":"error","msg":"Process did not exit before 15s grace period","severity":"error","time":"2022-06-14T13:59:23Z"}

pierDipi · 2022-06-14T14:05:18Z

/test channel-integration-tests-sasl-plain_eventing-kafka-broker_main

pierDipi · 2022-06-14T17:13:36Z

/test integration-tests_eventing-kafka-broker_main

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi · 2022-06-15T05:51:23Z

/retest-required

data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/metrics/Metrics.java

aliok

Just added a minor comment.

My knowledge on Vert.x is rather limited, so, LGTM since tests are green and the reproducer is fixed

Signed-off-by: Pierangelo Di Pilato <[email protected]>

aliok · 2022-06-15T08:13:11Z

/lgtm
/approve

Thanks!

knative-prow · 2022-06-15T08:13:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, 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:

~~OWNERS~~ [aliok,pierDipi]

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

pierDipi · 2022-06-15T08:23:35Z

/test integration-tests_eventing-kafka-broker_main

pierDipi · 2022-06-15T10:39:31Z

/cherry-pick release-1.4

pierDipi · 2022-06-15T10:39:35Z

/cherry-pick release-1.5

knative-prow-robot · 2022-06-15T10:40:07Z

@pierDipi: #2300 failed to apply on top of branch "release-1.4":

Applying: Add reproducer as test
Applying: WIP fix
Using index info to reconstruct a base tree...
M	data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/AsyncCloseable.java
M	data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/metrics/Metrics.java
M	data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/reconciler/impl/ResourcesReconcilerMessageHandler.java
M	data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/BaseConsumerVerticle.java
M	data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OrderedConsumerVerticle.java
Falling back to patching base and 3-way merge...
Auto-merging data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OrderedConsumerVerticle.java
CONFLICT (content): Merge conflict in data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OrderedConsumerVerticle.java
Auto-merging data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/BaseConsumerVerticle.java
Auto-merging data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/reconciler/impl/ResourcesReconcilerMessageHandler.java
CONFLICT (content): Merge conflict in data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/reconciler/impl/ResourcesReconcilerMessageHandler.java
Auto-merging data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/metrics/Metrics.java
CONFLICT (content): Merge conflict in data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/metrics/Metrics.java
Auto-merging data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/AsyncCloseable.java
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 WIP fix
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2022-06-15T10:40:10Z

@pierDipi: #2300 failed to apply on top of branch "release-1.5":

Applying: Add reproducer as test
Applying: WIP fix
Using index info to reconstruct a base tree...
M	data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OrderedConsumerVerticle.java
Falling back to patching base and 3-way merge...
Auto-merging data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OrderedConsumerVerticle.java
CONFLICT (content): Merge conflict in data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OrderedConsumerVerticle.java
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 WIP fix
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

* Add reproducer as test Signed-off-by: Pierangelo Di Pilato <[email protected]> * WIP fix Signed-off-by: Pierangelo Di Pilato <[email protected]> * Use for loop in reproducer Signed-off-by: Pierangelo Di Pilato <[email protected]> * Fix reproducer script path Signed-off-by: Pierangelo Di Pilato <[email protected]> * Handle closed sender Signed-off-by: Pierangelo Di Pilato <[email protected]> * Fix unit tests Signed-off-by: Pierangelo Di Pilato <[email protected]> * Describe KafkaChannel and Subscription for debugging Signed-off-by: Pierangelo Di Pilato <[email protected]> * Move reproducer to test location and use a smaller number Signed-off-by: Pierangelo Di Pilato <[email protected]> * Revert meterBinderExecutor thread pool changes Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi force-pushed the SRVKE-1237 branch from a2974c7 to 8249a50 Compare June 14, 2022 10:29

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2022

pierDipi force-pushed the SRVKE-1237 branch from 8249a50 to fc57b80 Compare June 14, 2022 11:58

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2022

pierDipi force-pushed the SRVKE-1237 branch from fc57b80 to baa4de2 Compare June 14, 2022 14:53

pierDipi changed the title ~~[WIP] Run custom KafkaChannel test~~ [WIP] Dispatcher verticles are now worker verticles Jun 14, 2022

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2022

pierDipi added 3 commits June 14, 2022 20:18

Add reproducer as test

c537354

Signed-off-by: Pierangelo Di Pilato <[email protected]>

WIP fix

ad6aa0c

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Use for loop in reproducer

c2d5486

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi added 5 commits June 14, 2022 20:19

Fix reproducer script path

b453727

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Handle closed sender

104edf6

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Fix unit tests

cb29433

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Describe KafkaChannel and Subscription for debugging

9ae922a

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Move reproducer to test location and use a smaller number

bd48b6e

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi force-pushed the SRVKE-1237 branch from baa4de2 to bd48b6e Compare June 14, 2022 18:19

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2022

pierDipi changed the title ~~[WIP] Dispatcher verticles are now worker verticles~~ Dispatcher verticles are now worker verticles Jun 15, 2022

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 Jun 15, 2022

aliok reviewed Jun 15, 2022

View reviewed changes

data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/metrics/Metrics.java Outdated Show resolved Hide resolved

aliok reviewed Jun 15, 2022

View reviewed changes

data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/metrics/Metrics.java Outdated Show resolved Hide resolved

aliok reviewed Jun 15, 2022

View reviewed changes

Revert meterBinderExecutor thread pool changes

f61381e

Signed-off-by: Pierangelo Di Pilato <[email protected]>

knative-prow bot assigned aliok Jun 15, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2022

knative-prow bot merged commit b3d50ad into knative-extensions:main Jun 15, 2022

pierDipi deleted the SRVKE-1237 branch June 15, 2022 10:39

pierDipi mentioned this pull request Jul 6, 2022

[release-1.5] Dispatcher verticles are now worker verticles (#2300) #2383

Merged

pierDipi restored the SRVKE-1237 branch August 5, 2024 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dispatcher verticles are now worker verticles #2300

Dispatcher verticles are now worker verticles #2300

pierDipi commented Jun 13, 2022 •

edited

Loading

pierDipi commented Jun 13, 2022

pierDipi commented Jun 13, 2022

pierDipi commented Jun 13, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

codecov bot commented Jun 14, 2022 •

edited

Loading

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 15, 2022

aliok left a comment

aliok commented Jun 15, 2022

knative-prow bot commented Jun 15, 2022

pierDipi commented Jun 15, 2022

pierDipi commented Jun 15, 2022

pierDipi commented Jun 15, 2022

knative-prow-robot commented Jun 15, 2022

knative-prow-robot commented Jun 15, 2022

Dispatcher verticles are now worker verticles #2300

Dispatcher verticles are now worker verticles #2300

Conversation

pierDipi commented Jun 13, 2022 • edited Loading

pierDipi commented Jun 13, 2022

pierDipi commented Jun 13, 2022

pierDipi commented Jun 13, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

codecov bot commented Jun 14, 2022 • edited Loading

Codecov Report

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 14, 2022

pierDipi commented Jun 15, 2022

aliok left a comment

Choose a reason for hiding this comment

aliok commented Jun 15, 2022

knative-prow bot commented Jun 15, 2022

pierDipi commented Jun 15, 2022

pierDipi commented Jun 15, 2022

pierDipi commented Jun 15, 2022

knative-prow-robot commented Jun 15, 2022

knative-prow-robot commented Jun 15, 2022

pierDipi commented Jun 13, 2022 •

edited

Loading

codecov bot commented Jun 14, 2022 •

edited

Loading