Distributed KafkaChannel Sarama Client Refactor #297
Conversation
knative-prow-robot
added
the
approved
knative-prow-robot
added
the
size/XXL
Codecov Report
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 75.76% 76.08% +0.32%
==========================================
Files 116 114 -2
Lines 4555 4391 -164
==========================================
- Hits 3451 3341 -110
+ Misses 900 851 -49
+ Partials 204 199 -5
Continue to review full report at Codecov.
|
pkg/channel/distributed/common/kafka/admin/eventhub/adminclient.go
Outdated
Show resolved
Hide resolved
pkg/channel/distributed/common/kafka/admin/eventhub/adminclient.go
Outdated
Show resolved
Hide resolved
pkg/channel/distributed/common/kafka/admin/eventhub/adminclient.go
Outdated
Show resolved
Hide resolved
| defer producertesting.RestoreNewSyncProducerFn() | ||
|
|
||
| // Create Producer To Test | ||
| producer := createTestProducer(t, brokers, config, mockSyncProducer) |
There was a problem hiding this comment.
This test is doing exactly the same thing that TestNewProducer() is doing (except for the assert.False). Do we need TestNewProducer() as a separate test instead of just verifying "!mockSyncProducer.Closed()" here ? On the other hand, it's not hurting anything.
There was a problem hiding this comment.
Yeah, I've had them combined before (removed the test) and it can sometimes get messy when you want to verify different things... You are right about similarity but I'd like to leave it this way for now.
|
@travis-minke-sap I couldn't check every change but overall the changes look good. I would like to get this merged before I start working on #298 Could you fix the linting issue please? |
|
/assign |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eric-sap, matzew, travis-minke-sap 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 |
…#297) Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
…#297) Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
…#297) (knative-extensions#301) Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
This PR is a comprehensive refactoring of the Sarama clients (admin / producer / consumer) used in the
distributedKafkaChannel implementation. The reason for the refactor is to move closer towards an implementation that could be used by both Channel implementations. The work of further refining the implementation, moving to pkg/common and using from theconsolidatedchannel is left for the future as it is not a trivial exercise.In general the creation of Sarama clients is not a complicated process so you might ask why would we need/want a common set of code for doing so. There are two main reasons...
distributedimplementation provides support for Azure EventHubs as well as a "custom" sidecar proxy for the Sarama AdminClient's ability to Create/Delete Kafka Topics. If this logic were common then theconsolidatedimplementation would instantly gain this capability as well.fakesormocksfor testing and is not coded against interfaces. Therefore thedistributedimplementation provides an Interface for the AdminClient usage as well as testing utilities (mocks/stubs) for all the Sarama clients.OK fine, but that doesn't explain why we need to refactor the current
distributedimplementation? The answer is that for historical reasons the implementation was dynamically loading Secrets containing Kafka Authentication and adding them to the Sarama.Config provided. This was done to support the "pooling" of Azure EventHub Authentication credentials in order to work around the Azure limitation of 10 EventHubs (Kafka Topics) per EventHub Namespace. This refactor removes this pooling capability in favor of a simple approach whereby the Sarama.Config struct is expected to be complete and no Kubernetes resources are loaded. This also allows thedistributedimplementation to align with recent changes around common configuration/authentication handling.Great, but why doesn't this PR go further and actually make the new implementation a reusable component in
pkg/common? That was was the original intent of this work, but... recent changes to theconsolidatedhave made the complicated. New code was added to List the ConsumerGroups and cache them in order to maintain KafkaChannel Status. Thedistributedchannel has a different architecture and is able to do this tracking inline. This means that theconsolidatedimplementation is now using theListConsumerGroups()from the Sarama.AdminClient which is cannot be supported by the Azure EventHubs (and likely the custom-sidecar) use cases. Therefore, it makes sense to tackle that problem (if desired) as a separate effort that might consider one of the following approaches...consolidatedimplementation to eliminate this incompatability.distributedimplementations AdminClientInterface and find a way to support it for EventHubs/Custom-Sidecar usage.Proposed Changes
Release Note