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

[0.24] KafkaChannel to init offsets before dispatcher (#886) #383

Merged
merged 2 commits into from
Oct 7, 2021
Merged

[0.24] KafkaChannel to init offsets before dispatcher (#886) #383

merged 2 commits into from
Oct 7, 2021

Conversation

aliok
Copy link

@aliok aliok commented Oct 4, 2021 •
edited
Loading

@aliok
[0.24] KafkaChannel to init offsets before dispatcher (knative-extens…
5c5b0cb 
…ions#886)

* Init offsets in Kafka channel controller - first iteration

* Do not check prober for subscription readiness

* Better code style

* Get rid of probing for checking subscription readiness

* Get rid of unused deps

* Ooops, fixed the tests

* Pass context along the functions, which will be necessary later

* Fix unit test

* Move partition retrieval into a separate function, which is going to be used later

* Check if offsets are initialized in the consumerFactory

* IDE "extracts" method LOL

* Make unit tests working

* Move MockClusterAdmin into the common package and reuse it

* Copy paste tests for CheckIfAllOffsetsInitialized

* Unify tests for CheckIfAllOffsetsInitialized and InitOffsets

* Separate tests for CheckIfAllOffsetsInitialized and InitOffsets

* Do not block main reconciliation thread for offset checking

* Remove last crumbs of probing

* Change log level for offset init message

* Move some consts to right place

* Rename checkOffsets.. func to WaitForOffsets...

* Rename consumerOffsetInitializer to ConsumerGroupOffsetsChecker

* Do not handle deleted topics or partitions when checking the offsets

* Copy the partitions array when retrieving partitions

* Address comments

* Separate client and clusteradminclient creation

* Stash kafka client

* Revert "Stash kafka client"

This reverts commit 4d57547

* Do not do any offset initialization when subscription is already marked ready
@openshift-ci openshift-ci bot added the approved label Oct 4, 2021
@pierDipi pierDipi mentioned this pull request Oct 4, 2021
Closed
@aliok
Copy link
Author

aliok commented Oct 4, 2021

/test 47-e2e-aws-ocp-47

That's not good 😢

TestChannelChain/KafkaChannel-messaging.knative.dev/v1beta1 (289.11s)

@matzew
Copy link
Member

matzew commented Oct 5, 2021

/test all (just to see what the 4.8 run gives us than again)

How about applying also https://github.com/knative-sandbox/eventing-kafka/pull/917/files in here?

@matzew
Copy link
Member

matzew commented Oct 5, 2021

ci/prow/48-e2e-aws-ocp-48 went green again... 🧐

@matzew
Copy link
Member

matzew commented Oct 5, 2021

all green...

again:

/test all

@aliok
Copy link
Author

aliok commented Oct 5, 2021

/test all

all green. testing again.

@matzew
Copy link
Member

matzew commented Oct 5, 2021

and now the 4.8 falls off ...

/test 48-e2e-aws-ocp-48

For sure some sort of race/flake here, still

@matzew
Copy link
Member

matzew commented Oct 5, 2021

Green...

/test all

@matzew
Copy link
Member

matzew commented Oct 6, 2021

Green...again

Another round:
/test all

@aliok aliok mentioned this pull request Oct 6, 2021
Closed
@aliok
Copy link
Author

aliok commented Oct 6, 2021

/test 48-e2e-aws-ocp-48
/test 48-e2e-aws-ocp-47

Both have

        Error: FAIL MATCHING: saw 1/2 matching events.

@aliok @pierDipi
[0.24] KafkaChannel dispatcher offset checking improvements (knative-…
6d6aa64 
…extensions#924)

* Change poll loop

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

* Change poll loop and requeue channel on failure

* Get rid of unused func

* Fix unit tests

Co-authored-by: Pierangelo Di Pilato <[email protected]>
@matzew
Copy link
Member

matzew commented Oct 7, 2021

green...

/test 48-e2e-aws-ocp-48
/test 48-e2e-aws-ocp-47

1 similar comment
@matzew
Copy link
Member

matzew commented Oct 7, 2021

green...

/test 48-e2e-aws-ocp-48
/test 48-e2e-aws-ocp-47

@matzew
Copy link
Member

matzew commented Oct 7, 2021

/test 47-e2e-aws-ocp-47

matzew
matzew approved these changes Oct 7, 2021
View reviewed changes
Copy link
Member

@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

@openshift-ci openshift-ci bot added the lgtm label Oct 7, 2021
@openshift-ci
Copy link

openshift-ci bot commented Oct 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

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

@openshift-merge-robot openshift-merge-robot merged commit 9799ea0 into openshift-knative:release-v0.24 Oct 7, 2021
@aliok aliok deleted the 024-midstream-backport-2 branch October 8, 2021 06:23
ReToCode pushed a commit to ReToCode/eventing-kafka that referenced this pull request Jan 3, 2023
@lionelvillard
slighly improve e2e doc (openshift-knative#383)
bf9d52f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matzew matzew matzew approved these changes

@markusthoemmes markusthoemmes Awaiting requested review from markusthoemmes

@mgencur mgencur Awaiting requested review from mgencur

Assignees

@matzew matzew

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aliok @matzew @openshift-merge-robot