Commit offset on consumer close #1463

pierDipi · 2021-11-11T06:34:54Z

When a consumer is closed, we want to commit the offsets for all
owned partitions.

Fixes (some flaky tests)

Proposed Changes

Commit offset on consumer close

Release Note

Commit offset on consumer close.

Docs

None

pierDipi · 2021-11-11T06:38:08Z

/cherry-pick release-1.0

knative-prow-robot · 2021-11-11T06:38:09Z

@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-11T06:38:16Z

/cherry-pick release-0.26

knative-prow-robot · 2021-11-11T06:38:17Z

@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-11T06:38:23Z

/cherry-pick release-0.25

knative-prow-robot · 2021-11-11T06:38:24Z

@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.

codecov · 2021-11-11T06:38:57Z

Codecov Report

Merging #1463 (04c7894) into main (d24bcb7) will increase coverage by 0.09%.
The diff coverage is 90.90%.

@@             Coverage Diff              @@
##               main    #1463      +/-   ##
============================================
+ Coverage     73.41%   73.51%   +0.09%     
- Complexity      575      585      +10     
============================================
  Files           100      101       +1     
  Lines          3942     3964      +22     
  Branches        160      165       +5     
============================================
+ Hits           2894     2914      +20     
- Misses          808      809       +1     
- Partials        240      241       +1

Flag	Coverage Δ
java-unittests	`82.26% <90.90%> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
...patcher/impl/consumer/OrderedConsumerVerticle.java	`87.03% <0.00%> (-3.35%)`	⬇️
...ive/eventing/kafka/broker/core/AsyncCloseable.java	`66.66% <100.00%> (+3.03%)`	⬆️
...a/broker/dispatcher/impl/RecordDispatcherImpl.java	`81.92% <100.00%> (ø)`
...dispatcher/impl/consumer/BaseConsumerVerticle.java	`93.75% <100.00%> (+0.64%)`	⬆️
...broker/dispatcher/impl/consumer/OffsetManager.java	`100.00% <100.00%> (ø)`
...receiver/impl/handler/MethodNotAllowedHandler.java	`100.00% <0.00%> (ø)`
...ver/impl/handler/ControlPlaneProbeRequestUtil.java	`100.00% <0.00%> (ø)`
...g/kafka/broker/receiver/impl/ReceiverVerticle.java	`97.67% <0.00%> (+0.23%)`	⬆️

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 d24bcb7...04c7894. Read the comment docs.

When a consumer is closed, we want to commit the offsets for all owned partitions. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>

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

matzew

nit - a bit of comments

.../src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OffsetManager.java

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

matzew

/lgtm
/approve

knative-prow-robot · 2021-11-11T10:18:26Z

[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

knative-prow-robot · 2021-11-11T11:15:20Z

@pierDipi: new pull request created: #1465

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-11T11:15:56Z

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

Applying: Commit offset on consumer close
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/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/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 on consumer close
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-11T11:16:31Z

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

Applying: Commit offset on consumer close
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/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/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 on consumer close
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.

matzew · 2021-11-11T13:57:06Z

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

So... it be ? 🤷‍♂️

pierDipi · 2021-11-11T14:00:10Z

I'll do it manually 😄

* Commit offset on consumer close When a consumer is closed, we want to commit the offsets for all owned partitions. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Comments Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>

…tensions#1478) * Commit offset on consumer close When a consumer is closed, we want to commit the offsets for all owned partitions. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Comments Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>

* Commit offset on consumer close When a consumer is closed, we want to commit the offsets for all owned partitions. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> * Comments Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> Co-authored-by: Pierangelo Di Pilato <pdipilat@redhat.com>

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

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

pierDipi force-pushed the SRVKE-1025_Commit-on-close branch 2 times, most recently from 6232bb4 to 18ccda2 Compare November 11, 2021 06:52

pierDipi added the kind/bug Categorizes issue or PR as related to a bug. label Nov 11, 2021

Commit offset on consumer close

e2bd9cf

When a consumer is closed, we want to commit the offsets for all owned partitions. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>

pierDipi force-pushed the SRVKE-1025_Commit-on-close branch from 18ccda2 to e2bd9cf Compare November 11, 2021 07:24

matzew reviewed Nov 11, 2021

View reviewed changes

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

matzew requested changes Nov 11, 2021

View reviewed changes

.../src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OffsetManager.java Show resolved Hide resolved

.../src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/OffsetManager.java Show resolved Hide resolved

knative-prow-robot assigned matzew Nov 11, 2021

Comments

04c7894

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

pierDipi requested a review from matzew November 11, 2021 08:49

matzew approved these changes Nov 11, 2021

View reviewed changes

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

knative-prow-robot merged commit 35d74b0 into knative-extensions:main Nov 11, 2021

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

[release-1.0] Commit offset on consumer close #1465

Merged

pierDipi deleted the SRVKE-1025_Commit-on-close branch November 11, 2021 11:31

pierDipi mentioned this pull request Nov 12, 2021

[release-0.26] Commit offset on consumer close (#1463) #1478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commit offset on consumer close #1463

Commit offset on consumer close #1463

pierDipi commented Nov 11, 2021

pierDipi commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

pierDipi commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

pierDipi commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

codecov bot commented Nov 11, 2021 •

edited

Loading

matzew left a comment

matzew left a comment

knative-prow-robot commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

matzew commented Nov 11, 2021

pierDipi commented Nov 11, 2021

Commit offset on consumer close #1463

Commit offset on consumer close #1463

Conversation

pierDipi commented Nov 11, 2021

Proposed Changes

pierDipi commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

pierDipi commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

pierDipi commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

codecov bot commented Nov 11, 2021 • edited Loading

Codecov Report

matzew left a comment

Choose a reason for hiding this comment

matzew left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

knative-prow-robot commented Nov 11, 2021

matzew commented Nov 11, 2021

pierDipi commented Nov 11, 2021

codecov bot commented Nov 11, 2021 •

edited

Loading