Add ConsumerGroup scheduling #1632

pierDipi · 2021-12-17T15:33:08Z

Part of #1537

Proposed Changes

Add ConsumerGroup scheduling

Release Note

None

Docs

None

codecov · 2021-12-17T15:37:01Z

Codecov Report

Merging #1632 (59cbd86) into main (5fcac54) will increase coverage by 0.77%.
The diff coverage is 68.83%.

@@             Coverage Diff              @@
##               main    #1632      +/-   ##
============================================
+ Coverage     71.86%   72.64%   +0.77%     
- Complexity      595      597       +2     
============================================
  Files           122      122              
  Lines          4521     4569      +48     
  Branches        171      171              
============================================
+ Hits           3249     3319      +70     
+ Misses         1008      977      -31     
- Partials        264      273       +9

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

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

Impacted Files	Coverage Δ
...afka/eventing/v1alpha1/consumer_group_lifecycle.go	`0.00% <0.00%> (ø)`
...ls/kafka/eventing/v1alpha1/consumer_group_types.go	`0.00% <0.00%> (ø)`
...nternals/kafka/eventing/v1alpha1/consumer_types.go	`0.00% <ø> (ø)`
...apis/internals/kafka/eventing/v1alpha1/register.go	`0.00% <ø> (ø)`
...lane/pkg/reconciler/consumergroup/consumergroup.go	`76.82% <80.30%> (+76.82%)`	⬆️
control-plane/pkg/prober/cache.go	`100.00% <0.00%> (+4.76%)`	⬆️
...dispatcher/impl/consumer/BaseConsumerVerticle.java	`93.75% <0.00%> (+6.25%)`	⬆️
control-plane/pkg/prober/prober.go	`75.00% <0.00%> (+15.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 5fcac54...59cbd86. Read the comment docs.

pierDipi · 2022-01-04T11:26:43Z

Flaky
/retest

pierDipi · 2022-01-07T07:33:00Z

Can I please get a review of this PR? @matzew @aliok @devguyio @aavarghese @lionelvillard

control-plane/pkg/apis/internals/kafka/eventing/v1alpha1/consumer_group_types.go

control-plane/pkg/reconciler/consumergroup/consumergroup.go

matzew · 2022-01-07T08:47:31Z

control-plane/pkg/reconciler/consumergroup/consumergroup.go


-		if _, err := r.InternalsClient.Consumers(cg.GetNamespace()).Create(ctx, c, metav1.CreateOptions{}); err != nil {
-			return fmt.Errorf("failed to create consumer %s/%s: %w", c.GetNamespace(), c.GetName(), err)
+	// Check if there is a consumer for the given placement.


if there is no consumer, we create/reconcile an internal consumer and return it ?

matzew · 2022-01-07T08:48:36Z

control-plane/pkg/reconciler/consumergroup/consumergroup.go

+	}
+
+	// Stable sort consumers so that we give consumers different deletion
+	// priorities based on their state.


I may lack context, but what state? the IsLessThan compares what?

I'll make this part more explicit in a follow-up PR.

IsLessThan defines the order for deleting consumers in the same placement.

Imagine there are consumer c1 and consumer c2 in pod p1.
We want to have only one consumer for a placement, so we remove one of them based on their state.
In our case, we're considering readiness or the creation time as a state, meaning that we try to keep ready consumers over not ready ones or if they aren't ready we remove the most recently created ones.

matzew · 2022-01-07T08:49:50Z

I gave a first review. any more to read for better context ?

pierDipi · 2022-01-07T08:56:30Z

The scheduler doc and API may be helpful: https://github.com/knative/eventing/tree/main/pkg/scheduler

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

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

knative-metrics-robot · 2022-01-10T07:52:00Z

The following is the coverage report on the affected files.
Say /test pull-knative-sandbox-eventing-kafka-broker-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
control-plane/pkg/reconciler/consumergroup/consumergroup.go	0.0%	87.1%	87.1

matzew · 2022-01-10T09:55:43Z

/lgtm
thanks or addressing my comments. +1 on the follow up PR

/hold
waiting for final approval from @aavarghese and/or @lionelvillard

aavarghese · 2022-01-10T21:39:25Z

control-plane/pkg/reconciler/testing/objects_consumer.go

+	}
+}
+
+func ConsumerTopics(topics ...string) ConsumerSpecOption {


For consistency, can some of these funcs be renamed to WithConsumerTopics, WithConsumerPlacement, WithConsumerConfigs, WithConsumerBootstrapServersConfig, etc. Wdyt?

aavarghese

All looks good! Func name change not critical for this iteration so..
/lgtm

knative-prow-robot · 2022-01-10T22:38:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aavarghese, 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~~ [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-01-10T22:44:40Z

Thanks!

/unhold

pierDipi force-pushed the SRVKE-1060-ConsumerGroup-scheduling branch 4 times, most recently from b1fe326 to 87933be Compare December 17, 2021 17:46

pierDipi force-pushed the SRVKE-1060-ConsumerGroup-scheduling branch from 87933be to de48c2f Compare January 4, 2022 09:26

pierDipi changed the title ~~[WIP] Add ConsumerGroup scheduling~~ Add ConsumerGroup scheduling Jan 4, 2022

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 Jan 4, 2022

pierDipi requested review from aavarghese, matzew, aliok and devguyio January 4, 2022 09:26

pierDipi force-pushed the SRVKE-1060-ConsumerGroup-scheduling branch from de48c2f to 4cc955a Compare January 7, 2022 07:32