Set subscription readiness based on consumer group status #182

devguyio · 2020-11-11T11:31:17Z

Fixes #134
Fixes #98
Fixes knative/eventing-contrib#1556

Proposed Changes

Set subscription readiness based on consumer group status in the consolidated Kafka
Set's KafkaChannel consolidated dispatcher to read-only
Enables HA in KafkaChannel consolidated dispatcher

Release Note

- A bug was fixed in the consolidated KafkaChannel where subscriptions would show up in the channel's `status.subscribers` before the dispatcher becomes ready to dispatch messages for those subscribers.
- Consolidated KafkaChannel dispatcher's horizontal scalability works now seamlessly with reconciler leader election.

pkg/channel/consolidated/reconciler/controller/kafkachannel.go

pkg/channel/consolidated/reconciler/controller/kafkawatcher.go

pkg/channel/consolidated/reconciler/controller/kafkachannel.go

codecov · 2020-11-12T20:27:53Z

Codecov Report

Merging #182 (d2d1e09) into master (e71a1b1) will decrease coverage by 0.77%.
The diff coverage is 62.58%.

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
- Coverage   75.99%   75.21%   -0.78%     
==========================================
  Files         112      114       +2     
  Lines        4274     4459     +185     
==========================================
+ Hits         3248     3354     +106     
- Misses        831      901      +70     
- Partials      195      204       +9

Impacted Files	Coverage Δ
...consolidated/reconciler/controller/kafkachannel.go	`43.43% <28.84%> (-2.97%)`	⬇️
pkg/channel/consolidated/kafka/admin.go	`79.41% <79.41%> (ø)`
...ed/reconciler/controller/consumer_group_watcher.go	`79.71% <79.71%> (ø)`
...annel/distributed/common/metrics/stats_reporter.go	`25.00% <0.00%> (-45.84%)`	⬇️
...el/distributed/dispatcher/dispatcher/dispatcher.go	`68.08% <0.00%> (-7.45%)`	⬇️
pkg/apis/bindings/v1beta1/kafka_types.go	`100.00% <0.00%> (ø)`
pkg/apis/bindings/v1alpha1/kafka_types.go	`100.00% <0.00%> (ø)`
pkg/apis/bindings/v1beta1/kafka_lifecycle.go	`50.00% <0.00%> (ø)`
pkg/apis/bindings/v1alpha1/kafka_lifecycle.go	`50.00% <0.00%> (ø)`
...rce/reconciler/source/resources/receive_adapter.go	`100.00% <0.00%> (ø)`
... and 5 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 e71a1b1...d2d1e09. Read the comment docs.

pkg/channel/consolidated/kafka/admin.go

pkg/channel/consolidated/reconciler/controller/kafkachannel.go

matzew · 2020-11-30T10:50:24Z

/lgtm

slinkydeveloper · 2020-11-30T11:54:10Z

/lgtm
robust work!

pkg/channel/consolidated/reconciler/controller/kafkachannel.go

pkg/channel/consolidated/reconciler/controller/consumer_group_watcher.go

pierDipi · 2020-11-30T12:37:41Z

Very nice! I want this to become a library usable by the Broker impl as well! (not in this PR)

lionelvillard · 2020-11-30T14:23:49Z

cmd/channel/consolidated/dispatcher/main.go

@@ -35,8 +35,5 @@ func main() {
 		ctx = injection.WithNamespaceScope(ctx, ns)
 	}

-	// Do not run the dispatcher in leader-election mode
-	ctx = sharedmain.WithHADisabled(ctx)
-


are you suggesting that we keep leader election disabled since this is just a read only reconciler? are there down sides of enabling it and observing how it behaves in this mode? since now we set up the consumers on all replicas with ObserveKind and on the leader with ReconcileKind

The end result is the same since we're doing the same thing in both ObserveKind and ReconcileKind but we don't really need to have replicas competing to acquire locks.

my understanding is that the preference is to have HA enabled, with ReconcileKind and ObserveKind , over disabling HA and having replicas with ReconcileKind since this works as intended. I might be mistaken. In all cases, I'd appreciate if this detail doesn't stop the merging of the PR if it's functionally correct.

I'm working on implementing a mix of ReconcileKind and ObserveKind in the dispatcher in a PoC as was suggested here knative/eventing-contrib#1446 (comment) , so I need HA eventually anyway.

we don't really need to have replicas competing to acquire locks.

That. Allowing HA seems confusing to me as there is no need it.

* Use sets.String for internal cache instead of string slice * Allow for a single callback per watcher * Synchronization on Terminate and Forget

devguyio · 2020-12-01T11:51:59Z

@pierDipi @matzew all comments addressed

knative-metrics-robot · 2020-12-01T11:53:43Z

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

File	Old Coverage	New Coverage	Delta
pkg/channel/consolidated/kafka/admin.go	Do not exist	84.6%
pkg/channel/consolidated/reconciler/controller/consumer_group_watcher.go	Do not exist	81.5%
pkg/channel/consolidated/reconciler/controller/kafkachannel.go	53.5%	52.0%	-1.5

pierDipi · 2020-12-01T12:04:57Z

cmd/channel/consolidated/dispatcher/main.go

@@ -35,8 +35,5 @@ func main() {
 		ctx = injection.WithNamespaceScope(ctx, ns)
 	}

-	// Do not run the dispatcher in leader-election mode
-	ctx = sharedmain.WithHADisabled(ctx)
-


The end result is the same since we're doing the same thing in both ObserveKind and ReconcileKind but we don't really need to have replicas competing to acquire locks.

pierDipi · 2020-12-01T12:12:30Z

pkg/channel/consolidated/reconciler/controller/kafkachannel.go

 func (r *Reconciler) FinalizeKind(ctx context.Context, kc *v1beta1.KafkaChannel) pkgreconciler.Event {
 	// Do not attempt retrying creating the client because it might be a permanent error
 	// in which case the finalizer will never get removed.
-	if kafkaClusterAdmin, err := r.createClient(ctx, kc); err == nil && r.kafkaConfig != nil {
+	if kafkaClusterAdmin, err := r.createClient(); err == nil && r.kafkaConfig != nil {
 		if err := r.deleteTopic(ctx, kc, kafkaClusterAdmin); err != nil {
 			return err
 		}
 	}
+	r.consumerGroupWatcher.Forget(string(kc.ObjectMeta.UID))
 	return newReconciledNormal(kc.Namespace, kc.Name) //ok to remove finalizer
 }


This won't be called on non-leader replicas, so to properly clean up resources we have to implement ReadOnlyFinalizer or keep HA disabled and have only ReconcileKind and FinalizeKind.
I think the latter solution is better (less code, no wasted resource to acquire locks, etc).

@pierDipi sorry I got a little confused, this is the controller not the dispatcher. The controller runs in HA with a single leader, and stand-by replicas. We don't need this called on non-leader replicas cause they don't set watchers first place.

I was confused too :)

But I think the comment is valid (in the wrong place) since the dispatcher doesn't implement Finalizer at all, and that's a bug that we can address later.

Opened an issue: #245

pierDipi

/lgtm

slinkydeveloper · 2020-12-01T12:40:27Z

/approve

knative-prow-robot · 2020-12-01T12:40:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devguyio, pierDipi, slinkydeveloper

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~~ [slinkydeveloper]

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

Set subscription readiness based on consumer group status (knative#182) * WIP: Set subscription readiness based on consumer group status * Refactor KafkaWatcher to an edge-triggered, level driven design * Refactor the test to prevent a map race * goimports and gofmt comments * Add tests for the KafkaWatcher and reconnect for AdminClient * Improve consumer group watcher code aethetics and robustness * Make variable names more descriptive for consumer group watcher and adimn client * Forget watcher when channel is deleted * Terminate old watcher and free the cache when config is created * Set the KafkaChannel consolidated dispatcher reconciler to be read-only * Enable HA for KafkaChannel(consolidated) dispatcher * Run update-codegen.sh * Optimize consumer group watcher * Use sets.String for internal cache instead of string slice * Allow for a single callback per watcher * Synchronization on Terminate and Forget

Set subscription readiness based on consumer group status (#182) * WIP: Set subscription readiness based on consumer group status * Refactor KafkaWatcher to an edge-triggered, level driven design * Refactor the test to prevent a map race * goimports and gofmt comments * Add tests for the KafkaWatcher and reconnect for AdminClient * Improve consumer group watcher code aethetics and robustness * Make variable names more descriptive for consumer group watcher and adimn client * Forget watcher when channel is deleted * Terminate old watcher and free the cache when config is created * Set the KafkaChannel consolidated dispatcher reconciler to be read-only * Enable HA for KafkaChannel(consolidated) dispatcher * Run update-codegen.sh * Optimize consumer group watcher * Use sets.String for internal cache instead of string slice * Allow for a single callback per watcher * Synchronization on Terminate and Forget

matzew · 2020-12-02T09:05:50Z

@devguyio can you backport this to the release-0.19 branch ?

…tensions#182) * WIP: Set subscription readiness based on consumer group status * Refactor KafkaWatcher to an edge-triggered, level driven design * Refactor the test to prevent a map race * goimports and gofmt comments * Add tests for the KafkaWatcher and reconnect for AdminClient * Improve consumer group watcher code aethetics and robustness * Make variable names more descriptive for consumer group watcher and adimn client * Forget watcher when channel is deleted * Terminate old watcher and free the cache when config is created * Set the KafkaChannel consolidated dispatcher reconciler to be read-only * Enable HA for KafkaChannel(consolidated) dispatcher * Run update-codegen.sh * Optimize consumer group watcher * Use sets.String for internal cache instead of string slice * Allow for a single callback per watcher * Synchronization on Terminate and Forget

* WIP: Set subscription readiness based on consumer group status * Refactor KafkaWatcher to an edge-triggered, level driven design * Refactor the test to prevent a map race * goimports and gofmt comments * Add tests for the KafkaWatcher and reconnect for AdminClient * Improve consumer group watcher code aethetics and robustness * Make variable names more descriptive for consumer group watcher and adimn client * Forget watcher when channel is deleted * Terminate old watcher and free the cache when config is created * Set the KafkaChannel consolidated dispatcher reconciler to be read-only * Enable HA for KafkaChannel(consolidated) dispatcher * Run update-codegen.sh * Optimize consumer group watcher * Use sets.String for internal cache instead of string slice * Allow for a single callback per watcher * Synchronization on Terminate and Forget

…tensions#182) (knative-extensions#249) * WIP: Set subscription readiness based on consumer group status * Refactor KafkaWatcher to an edge-triggered, level driven design * Refactor the test to prevent a map race * goimports and gofmt comments * Add tests for the KafkaWatcher and reconnect for AdminClient * Improve consumer group watcher code aethetics and robustness * Make variable names more descriptive for consumer group watcher and adimn client * Forget watcher when channel is deleted * Terminate old watcher and free the cache when config is created * Set the KafkaChannel consolidated dispatcher reconciler to be read-only * Enable HA for KafkaChannel(consolidated) dispatcher * Run update-codegen.sh * Optimize consumer group watcher * Use sets.String for internal cache instead of string slice * Allow for a single callback per watcher * Synchronization on Terminate and Forget

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 11, 2020

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

knative-prow-robot requested review from phamilton and travis-minke-sap November 11, 2020 11:31

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 11, 2020

lionelvillard reviewed Nov 11, 2020

View reviewed changes

pkg/channel/consolidated/reconciler/controller/kafkachannel.go Outdated Show resolved Hide resolved

lionelvillard reviewed Nov 11, 2020

View reviewed changes

pkg/channel/consolidated/reconciler/controller/kafkawatcher.go Outdated Show resolved Hide resolved

lionelvillard reviewed Nov 11, 2020

View reviewed changes

pkg/channel/consolidated/reconciler/controller/kafkachannel.go Outdated Show resolved Hide resolved

lionelvillard reviewed Nov 11, 2020

View reviewed changes

pkg/channel/consolidated/reconciler/controller/kafkachannel.go Outdated Show resolved Hide resolved

devguyio mentioned this pull request Nov 13, 2020

KafkaChannel subscribers list ends up with duplicate subscribers for the same subscription. This results in duplicate dispatches. knative/eventing-contrib#1560

Closed

matzew reviewed Nov 13, 2020

View reviewed changes

pkg/channel/consolidated/kafka/admin.go Outdated Show resolved Hide resolved

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

devguyio force-pushed the cgs-sync branch from 17ca113 to f7f197b Compare November 20, 2020 21:00

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 20, 2020

matzew reviewed Nov 23, 2020

View reviewed changes

pkg/channel/consolidated/kafka/admin.go Outdated Show resolved Hide resolved

matzew reviewed Nov 23, 2020

View reviewed changes

pkg/channel/consolidated/kafka/admin.go Outdated Show resolved Hide resolved

devguyio changed the title ~~[WIP] [POC] Set subscription readiness based on consumer group status~~ [POC] Set subscription readiness based on consumer group status Nov 23, 2020

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 23, 2020

matzew reviewed Nov 23, 2020

View reviewed changes

pkg/channel/consolidated/kafka/admin.go Show resolved Hide resolved

devguyio changed the title ~~[POC] Set subscription readiness based on consumer group status~~ [WIP] Set subscription readiness based on consumer group status Nov 23, 2020

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 23, 2020

devguyio added 5 commits November 23, 2020 16:03

WIP: Set subscription readiness based on consumer group status

cf12f38

Refactor KafkaWatcher to an edge-triggered, level driven design

6aaf357

Refactor the test to prevent a map race

dde158c

goimports and gofmt comments

9ca953c

Add tests for the KafkaWatcher and reconnect for AdminClient

a33f071

matzew reviewed Nov 23, 2020

View reviewed changes

pkg/channel/consolidated/reconciler/controller/kafkachannel.go Outdated Show resolved Hide resolved

knative-prow-robot assigned matzew Nov 30, 2020

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

pierDipi reviewed Nov 30, 2020

View reviewed changes

lionelvillard reviewed Nov 30, 2020

View reviewed changes

Optimize consumer group watcher

d2d1e09

* Use sets.String for internal cache instead of string slice * Allow for a single callback per watcher * Synchronization on Terminate and Forget

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020

pierDipi reviewed Dec 1, 2020

View reviewed changes

pierDipi mentioned this pull request Dec 1, 2020

[Consolidated] Dispatcher should implement Finalizer to properly clean up resources #245

Closed

pierDipi approved these changes Dec 1, 2020

View reviewed changes

knative-prow-robot assigned pierDipi Dec 1, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2020

knative-prow-robot merged commit 87238dc into knative-extensions:master Dec 1, 2020

devguyio deleted the cgs-sync branch December 1, 2020 12:42

devguyio mentioned this pull request Dec 1, 2020

Set subscription readiness based on consumer group status knative/eventing-contrib#1672

Merged

devguyio mentioned this pull request Dec 2, 2020

Backport: Set subscription readiness based on consumer group status (#182) #249

Merged

devguyio mentioned this pull request Dec 16, 2020

[consolidated] Update channel's status.subscribers based on the dispatcher propagated consumer readiness #280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set subscription readiness based on consumer group status #182

Set subscription readiness based on consumer group status #182

devguyio commented Nov 11, 2020 •

edited

Loading

codecov bot commented Nov 12, 2020 •

edited

Loading

matzew commented Nov 30, 2020

slinkydeveloper commented Nov 30, 2020

pierDipi commented Nov 30, 2020 •

edited

Loading

lionelvillard Nov 30, 2020

devguyio Nov 30, 2020

pierDipi Dec 1, 2020 •

edited

Loading

devguyio Dec 1, 2020

lionelvillard Dec 1, 2020

devguyio commented Dec 1, 2020

knative-metrics-robot commented Dec 1, 2020

pierDipi Dec 1, 2020 •

edited

Loading

pierDipi Dec 1, 2020

devguyio Dec 1, 2020

pierDipi Dec 1, 2020

pierDipi Dec 1, 2020

pierDipi left a comment

slinkydeveloper commented Dec 1, 2020

knative-prow-robot commented Dec 1, 2020

matzew commented Dec 2, 2020

Set subscription readiness based on consumer group status #182

Set subscription readiness based on consumer group status #182

Conversation

devguyio commented Nov 11, 2020 • edited Loading

Proposed Changes

codecov bot commented Nov 12, 2020 • edited Loading

Codecov Report

matzew commented Nov 30, 2020

slinkydeveloper commented Nov 30, 2020

pierDipi commented Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierDipi Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devguyio commented Dec 1, 2020

knative-metrics-robot commented Dec 1, 2020

pierDipi Dec 1, 2020 • edited Loading

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

pierDipi left a comment

Choose a reason for hiding this comment

slinkydeveloper commented Dec 1, 2020

knative-prow-robot commented Dec 1, 2020

matzew commented Dec 2, 2020

devguyio commented Nov 11, 2020 •

edited

Loading

codecov bot commented Nov 12, 2020 •

edited

Loading

pierDipi commented Nov 30, 2020 •

edited

Loading

pierDipi Dec 1, 2020 •

edited

Loading

pierDipi Dec 1, 2020 •

edited

Loading