-
Notifications
You must be signed in to change notification settings - Fork 82
[KafkaSource] Use the control protocol to expose the consumer group status #328
Changes from all commits
a92730e
9b8930a
70be9e1
243b5bc
9fe88e9
644b257
c9ae344
68ba97a
bac78b1
dc95847
f4c79d5
3085f57
c8b91ea
6593efe
c7ea2d0
62fc1e1
3706388
a6f6b6a
cd63da0
eef54e4
2cb87b0
5ec97b9
be432d7
759cd92
ee56189
f133168
3bcb691
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,9 @@ import ( | |
|
||
"golang.org/x/time/rate" | ||
|
||
ctrl "knative.dev/control-protocol/pkg" | ||
ctrlnetwork "knative.dev/control-protocol/pkg/network" | ||
|
||
"github.com/Shopify/sarama" | ||
"go.opencensus.io/trace" | ||
"go.uber.org/zap" | ||
|
@@ -38,6 +41,7 @@ import ( | |
|
||
"knative.dev/eventing-kafka/pkg/common/consumer" | ||
"knative.dev/eventing-kafka/pkg/source/client" | ||
kafkasourcecontrol "knative.dev/eventing-kafka/pkg/source/control" | ||
) | ||
|
||
const ( | ||
|
@@ -59,7 +63,9 @@ func NewEnvConfig() adapter.EnvConfigAccessor { | |
} | ||
|
||
type Adapter struct { | ||
config *AdapterConfig | ||
config *AdapterConfig | ||
controlServer *ctrlnetwork.ControlServer | ||
|
||
httpMessageSender *kncloudevents.HTTPMessageSender | ||
reporter pkgsource.StatsReporter | ||
logger *zap.SugaredLogger | ||
|
@@ -68,6 +74,8 @@ type Adapter struct { | |
} | ||
|
||
var _ adapter.MessageAdapter = (*Adapter)(nil) | ||
var _ consumer.KafkaConsumerHandler = (*Adapter)(nil) | ||
var _ consumer.SaramaConsumerLifecycleListener = (*Adapter)(nil) | ||
var _ adapter.MessageAdapterConstructor = NewAdapter | ||
|
||
func NewAdapter(ctx context.Context, processed adapter.EnvConfigAccessor, httpMessageSender *kncloudevents.HTTPMessageSender, reporter pkgsource.StatsReporter) adapter.MessageAdapter { | ||
|
@@ -86,7 +94,7 @@ func (a *Adapter) GetConsumerGroup() string { | |
return a.config.ConsumerGroup | ||
} | ||
|
||
func (a *Adapter) Start(ctx context.Context) error { | ||
func (a *Adapter) Start(ctx context.Context) (err error) { | ||
a.logger.Infow("Starting with config: ", | ||
zap.String("Topics", strings.Join(a.config.Topics, ",")), | ||
zap.String("ConsumerGroup", a.config.ConsumerGroup), | ||
|
@@ -95,14 +103,27 @@ func (a *Adapter) Start(ctx context.Context) error { | |
zap.String("Namespace", a.config.Namespace), | ||
) | ||
|
||
// Init control service | ||
a.controlServer, err = ctrlnetwork.StartInsecureControlServer(ctx) | ||
if err != nil { | ||
return err | ||
} | ||
a.controlServer.MessageHandler(a) | ||
|
||
// init consumer group | ||
addrs, config, err := client.NewConfigWithEnv(context.Background(), &a.config.KafkaEnvConfig) | ||
if err != nil { | ||
return fmt.Errorf("failed to create the config: %w", err) | ||
} | ||
|
||
consumerGroupFactory := consumer.NewConsumerGroupFactory(addrs, config) | ||
group, err := consumerGroupFactory.StartConsumerGroup(a.config.ConsumerGroup, a.config.Topics, a.logger, a) | ||
group, err := consumerGroupFactory.StartConsumerGroup( | ||
a.config.ConsumerGroup, | ||
a.config.Topics, | ||
a.logger, | ||
a, | ||
consumer.WithSaramaConsumerLifecycleListener(a), | ||
) | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
@@ -177,3 +198,22 @@ func (a *Adapter) Handle(ctx context.Context, msg *sarama.ConsumerMessage) (bool | |
func (a *Adapter) SetRateLimits(r rate.Limit, b int) { | ||
a.rateLimiter = rate.NewLimiter(r, b) | ||
} | ||
|
||
func (a *Adapter) HandleServiceMessage(ctx context.Context, message ctrl.ServiceMessage) { | ||
// In this first PR, there is only the RA sending messages to control plane, | ||
// there is no message the control plane should send to the RA | ||
a.logger.Info("Received unexpected control message") | ||
message.Ack() | ||
} | ||
|
||
func (a *Adapter) Setup(sess sarama.ConsumerGroupSession) { | ||
if err := a.controlServer.SendAndWaitForAck(kafkasourcecontrol.NotifySetupClaimsOpCode, kafkasourcecontrol.Claims(sess.Claims())); err != nil { | ||
a.logger.Warnf("Cannot send the claims update: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these codecov warnings make me wonder, do we have a way to mock/unittest the control protocol as a consumer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recently developed some mocks and other testing utilities https://github.com/knative-sandbox/control-protocol/tree/main/pkg/test, but I still don't use these here. You want me to tackle it now or in a followup? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't you planning to start using more of that? e.g. for changing/updating topics? I am fine in handling it via a follow-up. |
||
} | ||
} | ||
|
||
func (a *Adapter) Cleanup(sess sarama.ConsumerGroupSession) { | ||
if err := a.controlServer.SendAndWaitForAck(kafkasourcecontrol.NotifyCleanupClaimsOpCode, kafkasourcecontrol.Claims(sess.Claims())); err != nil { | ||
a.logger.Warnf("Cannot send the claims update: %v", err) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
package control | ||
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
"strings" | ||
|
||
"k8s.io/apimachinery/pkg/util/sets" | ||
|
||
ctrl "knative.dev/control-protocol/pkg" | ||
) | ||
|
||
// This just contains the different opcodes | ||
const ( | ||
NotifySetupClaimsOpCode ctrl.OpCode = 1 | ||
NotifyCleanupClaimsOpCode ctrl.OpCode = 2 | ||
Comment on lines
+15
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we good with just single digits ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why shouldn't we be? |
||
) | ||
|
||
type Claims map[string][]int32 | ||
|
||
func ClaimsParser(payload []byte) (interface{}, error) { | ||
var claims Claims | ||
err := (&claims).UnmarshalBinary(payload) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return claims, nil | ||
} | ||
|
||
func ClaimsMerger(old interface{}, new interface{}) interface{} { | ||
oldClaims := old.(Claims) | ||
newClaims := new.(Claims) | ||
result := oldClaims.copy() | ||
|
||
for topic, partitions := range result { | ||
if newPartitions, ok := newClaims[topic]; ok { | ||
result[topic] = sets.NewInt32(partitions...).Insert(newPartitions...).List() // Merge partitions | ||
delete(newClaims, topic) | ||
} | ||
} | ||
for newTopic, newPartitions := range newClaims { | ||
result[newTopic] = newPartitions | ||
} | ||
|
||
return result | ||
} | ||
|
||
func ClaimsDifference(old interface{}, new interface{}) interface{} { | ||
oldClaims := old.(Claims) | ||
cleanedClaims := new.(Claims) | ||
result := oldClaims.copy() | ||
|
||
for topic, partitions := range result { | ||
if cleanedPartitions, ok := cleanedClaims[topic]; ok { | ||
newSet := sets.NewInt32(partitions...).Delete(cleanedPartitions...).List() | ||
if len(newSet) == 0 { | ||
delete(result, topic) | ||
} else { | ||
result[topic] = newSet | ||
} | ||
} | ||
} | ||
|
||
return result | ||
} | ||
|
||
func (c Claims) String() string { | ||
strs := make([]string, 0, len(c)) | ||
for topic, partitions := range c { | ||
strs = append(strs, fmt.Sprintf("'%s': %v", topic, partitions)) | ||
} | ||
return strings.Join(strs, ", ") | ||
} | ||
|
||
func (c Claims) MarshalBinary() (data []byte, err error) { | ||
return json.Marshal(c) | ||
} | ||
|
||
func (c *Claims) UnmarshalBinary(data []byte) error { | ||
return json.Unmarshal(data, c) | ||
} | ||
|
||
func (c Claims) copy() Claims { | ||
res := make(Claims, len(c)) | ||
|
||
for k, v := range c { | ||
res[k] = v | ||
} | ||
|
||
return res | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insecrure :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah for this first pass, we don't encrypt the connection with mTLS, I think the certificates controller still need some work.