Commit offsets at specified intervals #1405

pierDipi · 2021-11-03T18:54:39Z

Committing offsets at a specified interval reduces Kafka cluster
load and makes the event delivery hundreds of times faster.

auto.commit.interval.ms is used to determine the commit
interval.

Proposed Changes

Commit offsets at specified intervals

Release Note

Kafka Broker event delivery is hundreds of times faster

Docs

None

codecov · 2021-11-03T18:58:25Z

Codecov Report

Merging #1405 (b582863) into main (f8f2a9b) will decrease coverage by 5.04%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1405      +/-   ##
============================================
- Coverage     76.24%   71.19%   -5.05%     
+ Complexity      584      575       -9     
============================================
  Files            98       99       +1     
  Lines          3658     3906     +248     
  Branches        166      160       -6     
============================================
- Hits           2789     2781       -8     
- Misses          653      909     +256     
  Partials        216      216

Flag	Coverage Δ
java-unittests	`82.16% <100.00%> (-0.13%)`	⬇️

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

Impacted Files	Coverage Δ
...a/broker/dispatcher/impl/RecordDispatcherImpl.java	`81.92% <100.00%> (-0.64%)`	⬇️
...broker/dispatcher/impl/consumer/OffsetManager.java	`100.00% <100.00%> (ø)`
...r/dispatcher/main/ConsumerVerticleFactoryImpl.java	`88.07% <100.00%> (+1.58%)`	⬆️
control-plane/pkg/reconciler/kafka/topic.go	`97.67% <0.00%> (-2.33%)`	⬇️
control-plane/pkg/reconciler/broker/broker.go	`70.44% <0.00%> (ø)`
control-plane/pkg/reconciler/sink/kafka_sink.go	`67.83% <0.00%> (ø)`
...lane/pkg/reconciler/base/receiver_condition_set.go	`0.00% <0.00%> (ø)`
control-plane/pkg/reconciler/channel/channel.go	`0.00% <0.00%> (ø)`
control-plane/pkg/reconciler/channel/controller.go	`85.71% <0.00%> (ø)`

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 f8f2a9b...b582863. Read the comment docs.

pierDipi · 2021-11-04T10:25:26Z

artifacts.tar.gz

pierDipi · 2021-11-04T14:43:57Z

/cherry-pick release-1.0

knative-prow-robot · 2021-11-04T14:43:58Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.0 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.0

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.

pierDipi · 2021-11-04T14:47:14Z

/cherry-pick release-0.26

knative-prow-robot · 2021-11-04T14:47:15Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-0.26 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.26

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.

pierDipi · 2021-11-04T14:47:25Z

/cherry-pick release-0.25

knative-prow-robot · 2021-11-04T14:47:26Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-0.25 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.25

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.

matzew · 2021-11-04T17:02:26Z

...main/java/dev/knative/eventing/kafka/broker/dispatcher/main/ConsumerVerticleFactoryImpl.java

-      case UNORDERED -> new UnorderedOffsetManager(consumer, commitHandler);
-    };
-  }
-
  private static AbstractVerticle getConsumerVerticle(final DeliveryOrder type,


So, one Offset mgr is used for the (Un)Ordered Consumers - ?

Yes, there is no need to keep 2 separate components and implement the same commit interval logic in 2 places.

The offset tracker overhead in the ordered case is constant.

matzew · 2021-11-04T17:04:42Z

...main/java/dev/knative/eventing/kafka/broker/dispatcher/main/ConsumerVerticleFactoryImpl.java

@@ -178,14 +172,15 @@ public AbstractVerticle get(final DataPlaneContract.Resource resource, final Dat
          Filter.noop();

        final var responseHandler = getNoopResponseHandlerOrDefault(egress, () -> getKafkaResponseHandler(vertx, producerConfigs, resource));
+        final var commitIntervalMs = Integer.parseInt(String.valueOf(consumerConfigs.get(ConsumerConfig.AUTO_COMMIT_INTERVAL_MS_CONFIG)));


should we document this?

Sure, I'll open an issue in knative/docs.

Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>

.../knative/eventing/kafka/broker/dispatcher/impl/consumer/UnorderedOffsetManagerBenchmark.java

matzew · 2021-11-09T16:20:56Z

.../knative/eventing/kafka/broker/dispatcher/impl/consumer/UnorderedOffsetManagerBenchmark.java

-      blackhole.consume(
-        offsetManager.recordReceived(recordsState.records[partition][0])
-      );
+      offsetManager.recordReceived(recordsState.records[partition][0]);


are they directly consumed?

I updated the code to make it compile but I'm inclined to drop these OffsetManager benchmarks since they are not really useful to catch perf regressions as they are.

Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

matzew

/lgtm
/approve

matzew · 2021-11-09T16:27:07Z

...main/java/dev/knative/eventing/kafka/broker/dispatcher/main/ConsumerVerticleFactoryImpl.java

@@ -178,14 +172,15 @@ public AbstractVerticle get(final DataPlaneContract.Resource resource, final Dat
          Filter.noop();

        final var responseHandler = getNoopResponseHandlerOrDefault(egress, () -> getKafkaResponseHandler(vertx, producerConfigs, resource));
+        final var commitIntervalMs = Integer.parseInt(String.valueOf(consumerConfigs.get(ConsumerConfig.AUTO_COMMIT_INTERVAL_MS_CONFIG)));


knative-prow-robot · 2021-11-09T16:31:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, 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~~ [matzew,pierDipi]

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

matzew · 2021-11-09T16:31:59Z

@pierDipi should we backport this to 0.27 ? (and perhaps 0.26 - if possible) ?

pierDipi · 2021-11-09T16:32:47Z

Prow will do that #1405 (comment)

pierDipi · 2021-11-09T16:33:02Z

and #1405 (comment)

knative-prow-robot · 2021-11-09T18:33:24Z

@pierDipi: new pull request created: #1449

In response to this:

/cherry-pick release-1.0

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 · 2021-11-09T18:34:00Z

@pierDipi: #1405 failed to apply on top of branch "release-0.26":

Applying: Commit offset at a specified interval
Using index info to reconstruct a base tree...
M	data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java
M	data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/main/ConsumerVerticleFactoryImpl.java
M	data-plane/dispatcher/src/test/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherTest.java
M	data-plane/tests/src/test/java/dev/knative/eventing/kafka/broker/tests/DataPlaneTest.java
Falling back to patching base and 3-way merge...
Auto-merging data-plane/tests/src/test/java/dev/knative/eventing/kafka/broker/tests/DataPlaneTest.java
CONFLICT (content): Merge conflict in data-plane/tests/src/test/java/dev/knative/eventing/kafka/broker/tests/DataPlaneTest.java
Removing data-plane/dispatcher/src/test/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OrderedOffsetManagerTest.java
Auto-merging data-plane/dispatcher/src/test/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherTest.java
Auto-merging data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/main/ConsumerVerticleFactoryImpl.java
CONFLICT (content): Merge conflict in data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/main/ConsumerVerticleFactoryImpl.java
Removing data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/UnorderedOffsetManager.java
Removing data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OrderedOffsetManager.java
Auto-merging data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Commit offset at a specified interval
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-0.26

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 · 2021-11-09T18:34:35Z

@pierDipi: #1405 failed to apply on top of branch "release-0.25":

Applying: Commit offset at a specified interval
Using index info to reconstruct a base tree...
M	data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java
M	data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/main/ConsumerVerticleFactoryImpl.java
M	data-plane/dispatcher/src/test/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherTest.java
M	data-plane/tests/src/test/java/dev/knative/eventing/kafka/broker/tests/DataPlaneTest.java
Falling back to patching base and 3-way merge...
Auto-merging data-plane/tests/src/test/java/dev/knative/eventing/kafka/broker/tests/DataPlaneTest.java
CONFLICT (content): Merge conflict in data-plane/tests/src/test/java/dev/knative/eventing/kafka/broker/tests/DataPlaneTest.java
Removing data-plane/dispatcher/src/test/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OrderedOffsetManagerTest.java
Auto-merging data-plane/dispatcher/src/test/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherTest.java
Auto-merging data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/main/ConsumerVerticleFactoryImpl.java
CONFLICT (content): Merge conflict in data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/main/ConsumerVerticleFactoryImpl.java
Removing data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/UnorderedOffsetManager.java
Removing data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OrderedOffsetManager.java
Auto-merging data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Commit offset at a specified interval
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-0.25

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.

* Commit offset at a specified interval Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Add reset logic to handle long large offsets Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Use final Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

* Commit offsets at specified intervals (#1405) * Commit offset at a specified interval Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Add reset logic to handle long large offsets Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Use final Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> * Update codegen Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * ko upgrade fixes Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

* Commit offset at a specified interval Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Add reset logic to handle long large offsets Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Use final Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

…ons#1405) (knative-extensions#1450) * Commit offsets at specified intervals (knative-extensions#1405) * Commit offset at a specified interval Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Add reset logic to handle long large offsets Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Use final Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> * Update codegen Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * ko upgrade fixes Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

…1405) (#1454) * Commit offsets at specified intervals (#1405) * Commit offset at a specified interval Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Add reset logic to handle long large offsets Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Use final Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> * Update codegen Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * ko upgrade fixes Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> Co-authored-by: Pierangelo Di Pilato <pdipilat@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

…ons#1405) (knative-extensions#1450) * Commit offsets at specified intervals (knative-extensions#1405) * Commit offset at a specified interval Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Add reset logic to handle long large offsets Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Use final Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> * Update codegen Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * ko upgrade fixes Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 3, 2021

knative-prow-robot added area/data-plane size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 3, 2021

pierDipi force-pushed the SRVKE_Perf_commit-interval branch 7 times, most recently from d2ad17b to 73e28f8 Compare November 4, 2021 09:20

pierDipi changed the title ~~[WIP] [Ignore]~~ Commit offsets at specified intervals Nov 4, 2021

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2021

pierDipi changed the title ~~Commit offsets at specified intervals~~ [wip] Commit offsets at specified intervals Nov 4, 2021

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2021

pierDipi changed the title ~~[wip] Commit offsets at specified intervals~~ Commit offsets at specified intervals Nov 4, 2021

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2021

matzew reviewed Nov 4, 2021

View reviewed changes

pierDipi requested review from aliok and matzew November 5, 2021 08:01

pierDipi added 2 commits November 8, 2021 11:06

Commit offset at a specified interval

ea89ed0

Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>

Add reset logic to handle long large offsets

d2776e4

Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>

pierDipi force-pushed the SRVKE_Perf_commit-interval branch from 73e28f8 to d2776e4 Compare November 8, 2021 10:06

pierDipi changed the title ~~[wip] Commit offsets at specified intervals~~ Commit offsets at specified intervals Nov 8, 2021

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2021

matzew reviewed Nov 9, 2021

View reviewed changes

.../knative/eventing/kafka/broker/dispatcher/impl/consumer/UnorderedOffsetManagerBenchmark.java Outdated Show resolved Hide resolved

matzew reviewed Nov 9, 2021

View reviewed changes

Use final

b582863

Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

matzew approved these changes Nov 9, 2021

View reviewed changes

knative-prow-robot assigned matzew Nov 9, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2021

pierDipi mentioned this pull request Nov 9, 2021

Document Kafka Broker commit offset interval knative/docs#4460

Closed

knative-prow-robot merged commit 5a594f7 into knative-extensions:main Nov 9, 2021

knative-prow-robot mentioned this pull request Nov 9, 2021

[release-1.0] Commit offsets at specified intervals #1449

Merged

pierDipi deleted the SRVKE_Perf_commit-interval branch November 9, 2021 19:37

pierDipi mentioned this pull request Nov 9, 2021

[release-0.26] Commit offsets at specified intervals (#1405) #1450

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commit offsets at specified intervals #1405

Commit offsets at specified intervals #1405

pierDipi commented Nov 3, 2021 •

edited

Loading

codecov bot commented Nov 3, 2021 •

edited

Loading

pierDipi commented Nov 4, 2021

pierDipi commented Nov 4, 2021

knative-prow-robot commented Nov 4, 2021

pierDipi commented Nov 4, 2021

knative-prow-robot commented Nov 4, 2021

pierDipi commented Nov 4, 2021

knative-prow-robot commented Nov 4, 2021

matzew Nov 4, 2021

pierDipi Nov 5, 2021

matzew Nov 4, 2021

pierDipi Nov 5, 2021

matzew Nov 9, 2021

matzew Nov 9, 2021

pierDipi Nov 9, 2021

matzew left a comment

matzew Nov 9, 2021

knative-prow-robot commented Nov 9, 2021

matzew commented Nov 9, 2021

pierDipi commented Nov 9, 2021

pierDipi commented Nov 9, 2021

knative-prow-robot commented Nov 9, 2021

knative-prow-robot commented Nov 9, 2021

knative-prow-robot commented Nov 9, 2021

Commit offsets at specified intervals #1405

Commit offsets at specified intervals #1405

Conversation

pierDipi commented Nov 3, 2021 • edited Loading

Proposed Changes

codecov bot commented Nov 3, 2021 • edited Loading

Codecov Report

pierDipi commented Nov 4, 2021

pierDipi commented Nov 4, 2021

knative-prow-robot commented Nov 4, 2021

pierDipi commented Nov 4, 2021

knative-prow-robot commented Nov 4, 2021

pierDipi commented Nov 4, 2021

knative-prow-robot commented Nov 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matzew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 9, 2021

matzew commented Nov 9, 2021

pierDipi commented Nov 9, 2021

pierDipi commented Nov 9, 2021

knative-prow-robot commented Nov 9, 2021

knative-prow-robot commented Nov 9, 2021

knative-prow-robot commented Nov 9, 2021

pierDipi commented Nov 3, 2021 •

edited

Loading

codecov bot commented Nov 3, 2021 •

edited

Loading