-
Notifications
You must be signed in to change notification settings - Fork 51
kafka: release sarama clients on cleanup and init failures #4437
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.
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -27,8 +27,24 @@ import ( | |
| type saramaAdminClient struct { | ||
| changefeed common.ChangeFeedID | ||
|
|
||
| client sarama.Client | ||
| admin sarama.ClusterAdmin | ||
| // client is the underlying sarama client created for this admin wrapper. | ||
| // It must be closed to stop background goroutines (e.g. metadata updater) and release memory. | ||
| client saramaClient | ||
| admin saramaClusterAdmin | ||
| } | ||
|
|
||
| type saramaClient interface { | ||
|
Collaborator
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 2 interface looks similar to the admin interface.
Collaborator
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. It's not suggested to test on the sarama. |
||
| Brokers() []*sarama.Broker | ||
| Partitions(topic string) ([]int32, error) | ||
| Close() error | ||
| } | ||
|
|
||
| type saramaClusterAdmin interface { | ||
| DescribeCluster() (brokers []*sarama.Broker, controllerID int32, err error) | ||
| DescribeConfig(resource sarama.ConfigResource) ([]sarama.ConfigEntry, error) | ||
| DescribeTopics(topics []string) (metadata []*sarama.TopicMetadata, err error) | ||
| CreateTopic(topic string, detail *sarama.TopicDetail, validateOnly bool) error | ||
| Close() error | ||
| } | ||
|
|
||
| func (a *saramaAdminClient) GetAllBrokers() []Broker { | ||
|
|
@@ -172,10 +188,24 @@ func (a *saramaAdminClient) Heartbeat() { | |
| } | ||
|
|
||
| func (a *saramaAdminClient) Close() { | ||
| if err := a.admin.Close(); err != nil { | ||
| log.Warn("close admin client meet error", | ||
| zap.String("keyspace", a.changefeed.Keyspace()), | ||
| zap.String("changefeed", a.changefeed.Name()), | ||
| zap.Error(err)) | ||
| // For admins created via sarama.NewClusterAdminFromClient, admin.Close() takes care | ||
| // of closing the underlying client as well. Fall back to closing the client directly | ||
| // only when admin is unexpectedly nil. | ||
| if a.admin != nil { | ||
| if err := a.admin.Close(); err != nil { | ||
|
Collaborator
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. this call should already close the admin underline client. |
||
| log.Warn("close admin client meet error", | ||
| zap.String("keyspace", a.changefeed.Keyspace()), | ||
| zap.String("changefeed", a.changefeed.Name()), | ||
| zap.Error(err)) | ||
| } | ||
| return | ||
| } | ||
| if a.client != nil { | ||
| if err := a.client.Close(); err != nil { | ||
| log.Warn("close kafka client meet error", | ||
| zap.String("keyspace", a.changefeed.Keyspace()), | ||
| zap.String("changefeed", a.changefeed.Name()), | ||
| zap.Error(err)) | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| // Copyright 2026 PingCAP, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package kafka | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/IBM/sarama" | ||
| "github.com/pingcap/ticdc/pkg/common" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| type testSaramaClient struct { | ||
|
Collaborator
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. You should use gomock to generate a mocked admin client for the test purpose. |
||
| closed bool | ||
| } | ||
|
|
||
| func (c *testSaramaClient) Brokers() []*sarama.Broker { | ||
| return nil | ||
| } | ||
|
|
||
| func (c *testSaramaClient) Partitions(string) ([]int32, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| func (c *testSaramaClient) Close() error { | ||
| c.closed = true | ||
| return nil | ||
| } | ||
|
|
||
| type testSaramaClusterAdmin struct { | ||
| closed bool | ||
| closeClient *testSaramaClient | ||
| } | ||
|
|
||
| func (a *testSaramaClusterAdmin) DescribeCluster() ([]*sarama.Broker, int32, error) { | ||
| return nil, 0, nil | ||
| } | ||
|
|
||
| func (a *testSaramaClusterAdmin) DescribeConfig(sarama.ConfigResource) ([]sarama.ConfigEntry, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| func (a *testSaramaClusterAdmin) DescribeTopics([]string) ([]*sarama.TopicMetadata, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| func (a *testSaramaClusterAdmin) CreateTopic(string, *sarama.TopicDetail, bool) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (a *testSaramaClusterAdmin) Close() error { | ||
| a.closed = true | ||
| if a.closeClient != nil { | ||
| return a.closeClient.Close() | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func TestSaramaAdminClientCloseDelegatesClientCleanupToAdmin(t *testing.T) { | ||
| client := &testSaramaClient{} | ||
| admin := &testSaramaClusterAdmin{closeClient: client} | ||
| a := &saramaAdminClient{ | ||
| changefeed: common.NewChangeFeedIDWithName("test", "default"), | ||
| client: client, | ||
| admin: admin, | ||
| } | ||
| a.Close() | ||
| require.True(t, admin.closed) | ||
| require.True(t, client.closed) | ||
| } | ||
|
|
||
| func TestSaramaAdminClientCloseFallsBackToClientWhenAdminIsNil(t *testing.T) { | ||
| client := &testSaramaClient{} | ||
| a := &saramaAdminClient{ | ||
| changefeed: common.NewChangeFeedIDWithName("test", "default"), | ||
| client: client, | ||
| } | ||
| require.NotPanics(t, func() { a.Close() }) | ||
| require.True(t, client.closed) | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -88,6 +88,9 @@ func newAdminClient(changefeedID common.ChangeFeedID, endpoints []string, config | |||||||||||||||||||||||||||||||
| zap.Any("duration", duration), zap.Stringer("changefeedID", changefeedID)) | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||
| // `sarama.NewClusterAdminFromClient` does not take ownership of the client, | ||||||||||||||||||||||||||||||||
| // so we need to close it on failures to avoid leaking background goroutines. | ||||||||||||||||||||||||||||||||
| _ = client.Close() | ||||||||||||||||||||||||||||||||
|
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. |
||||||||||||||||||||||||||||||||
| return nil, errors.Trace(err) | ||||||||||||||||||||||||||||||||
|
Comment on lines
90
to
94
Contributor
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. Don't swallow the client-close failure on this cleanup path. If admin creation fails and Suggested change if err != nil {
// `sarama.NewClusterAdminFromClient` does not take ownership of the client,
// so we need to close it on failures to avoid leaking background goroutines.
- _ = client.Close()
+ if closeErr := client.Close(); closeErr != nil {
+ log.Warn("close kafka client after admin creation failed",
+ zap.Stringer("changefeedID", changefeedID),
+ zap.Error(closeErr))
+ }
return nil, errors.Trace(err)
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return &saramaAdminClient{ | ||||||||||||||||||||||||||||||||
|
|
@@ -122,6 +125,7 @@ func (f *saramaFactory) SyncProducer(ctx context.Context) (SyncProducer, error) | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| p, err := sarama.NewSyncProducerFromClient(client) | ||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||
| _ = client.Close() | ||||||||||||||||||||||||||||||||
| return nil, errors.WrapError(errors.ErrKafkaNewProducer, err) | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
126
to
130
Contributor
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. Log the client close error for consistency. Same concern as the admin creation path: silently discarding the close error loses visibility into potential cleanup failures. Suggested change p, err := sarama.NewSyncProducerFromClient(client)
if err != nil {
- _ = client.Close()
+ if closeErr := client.Close(); closeErr != nil {
+ log.Warn("close kafka client after sync producer creation failed",
+ zap.Stringer("changefeedID", f.changefeedID),
+ zap.Error(closeErr))
+ }
return nil, errors.WrapError(errors.ErrKafkaNewProducer, err)
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
@@ -149,6 +153,7 @@ func (f *saramaFactory) AsyncProducer(ctx context.Context) (AsyncProducer, error | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| p, err := sarama.NewAsyncProducerFromClient(client) | ||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||
| _ = client.Close() | ||||||||||||||||||||||||||||||||
| return nil, errors.WrapError(errors.ErrKafkaNewProducer, err) | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
154
to
158
Contributor
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. Log the client close error for consistency. Same pattern as other creation paths: log the close error rather than discarding it. Suggested change p, err := sarama.NewAsyncProducerFromClient(client)
if err != nil {
- _ = client.Close()
+ if closeErr := client.Close(); closeErr != nil {
+ log.Warn("close kafka client after async producer creation failed",
+ zap.Stringer("changefeedID", f.changefeedID),
+ zap.Error(closeErr))
+ }
return nil, errors.WrapError(errors.ErrKafkaNewProducer, err)
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
| return &saramaAsyncProducer{ | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,10 +26,21 @@ import ( | |
| "go.uber.org/zap" | ||
| ) | ||
|
|
||
| type saramaSyncClient interface { | ||
|
Collaborator
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. redundant interface。 |
||
| Brokers() []*sarama.Broker | ||
| Close() error | ||
| } | ||
|
|
||
| type saramaSyncProducerClient interface { | ||
| SendMessage(msg *sarama.ProducerMessage) (partition int32, offset int64, err error) | ||
| SendMessages(msgs []*sarama.ProducerMessage) error | ||
| Close() error | ||
| } | ||
|
|
||
| type saramaSyncProducer struct { | ||
| id commonType.ChangeFeedID | ||
| client sarama.Client | ||
| producer sarama.SyncProducer | ||
| client saramaSyncClient | ||
| producer saramaSyncProducerClient | ||
| closed *atomic.Bool | ||
| } | ||
|
|
||
|
|
@@ -110,15 +121,26 @@ func (p *saramaSyncProducer) Close() { | |
|
|
||
| p.closed.Store(true) | ||
| start := time.Now() | ||
| // this also close the client. | ||
| err := p.producer.Close() | ||
| if err != nil { | ||
| log.Error("Close Kafka DDL producer with error", | ||
| zap.String("keyspace", p.id.Keyspace()), | ||
| zap.String("changefeed", p.id.Name()), | ||
| zap.Duration("duration", time.Since(start)), | ||
| zap.Error(err)) | ||
| return | ||
| // sarama.NewSyncProducerFromClient wraps the provided client with a nopCloserClient, | ||
| // so producer.Close() alone won't release the underlying client resources. | ||
| if p.client != nil { | ||
| if err := p.client.Close(); err != nil { | ||
| log.Warn("Close Kafka DDL producer client with error", | ||
| zap.String("keyspace", p.id.Keyspace()), | ||
| zap.String("changefeed", p.id.Name()), | ||
| zap.Duration("duration", time.Since(start)), | ||
| zap.Error(err)) | ||
| } | ||
| } | ||
| if p.producer != nil { | ||
| if err := p.producer.Close(); err != nil { | ||
| log.Error("Close Kafka DDL producer with error", | ||
| zap.String("keyspace", p.id.Keyspace()), | ||
| zap.String("changefeed", p.id.Name()), | ||
| zap.Duration("duration", time.Since(start)), | ||
| zap.Error(err)) | ||
| return | ||
| } | ||
| } | ||
| log.Info("Kafka DDL producer closed", | ||
| zap.String("keyspace", p.id.Keyspace()), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| // Copyright 2026 PingCAP, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package kafka | ||
|
|
||
| import ( | ||
| "errors" | ||
| "testing" | ||
|
|
||
| "github.com/IBM/sarama" | ||
| "github.com/pingcap/ticdc/pkg/common" | ||
| "github.com/stretchr/testify/require" | ||
| "go.uber.org/atomic" | ||
| ) | ||
|
|
||
| type testSyncProducerClient struct { | ||
| closeCalls int | ||
| closeErr error | ||
| } | ||
|
|
||
| func (c *testSyncProducerClient) Brokers() []*sarama.Broker { | ||
| return nil | ||
| } | ||
|
|
||
| func (c *testSyncProducerClient) Close() error { | ||
| c.closeCalls++ | ||
| return c.closeErr | ||
| } | ||
|
|
||
| type testSaramaSyncProducer struct { | ||
| closeCalls int | ||
| } | ||
|
|
||
| func (p *testSaramaSyncProducer) SendMessage(*sarama.ProducerMessage) (int32, int64, error) { | ||
| return 0, 0, nil | ||
| } | ||
|
|
||
| func (p *testSaramaSyncProducer) SendMessages([]*sarama.ProducerMessage) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (p *testSaramaSyncProducer) Close() error { | ||
| p.closeCalls++ | ||
| return nil | ||
| } | ||
|
|
||
| func TestSaramaSyncProducerCloseClosesClientAndProducer(t *testing.T) { | ||
| client := &testSyncProducerClient{} | ||
| producer := &testSaramaSyncProducer{} | ||
| p := &saramaSyncProducer{ | ||
| id: common.NewChangeFeedIDWithName("test", "default"), | ||
| client: client, | ||
| producer: producer, | ||
| closed: atomic.NewBool(false), | ||
| } | ||
|
|
||
| p.Close() | ||
|
|
||
| require.Equal(t, 1, client.closeCalls) | ||
| require.Equal(t, 1, producer.closeCalls) | ||
| } | ||
|
|
||
| func TestSaramaSyncProducerCloseStillClosesProducerWhenClientCloseFails(t *testing.T) { | ||
| client := &testSyncProducerClient{closeErr: errors.New("boom")} | ||
| producer := &testSaramaSyncProducer{} | ||
| p := &saramaSyncProducer{ | ||
| id: common.NewChangeFeedIDWithName("test", "default"), | ||
| client: client, | ||
| producer: producer, | ||
| closed: atomic.NewBool(false), | ||
| } | ||
|
|
||
| p.Close() | ||
|
|
||
| require.Equal(t, 1, client.closeCalls) | ||
| require.Equal(t, 1, producer.closeCalls) | ||
| } |
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.
Don't add these Sarama goroutines to the global goleak allowlist.
These are the exact goroutines leaked when the admin wrapper forgets to close its underlying Sarama client. Putting them in
defaultOptswill make the suite ignore the regression this PR is trying to prevent. Please scope these ignores to the few tests that intentionally tolerate async producer shutdown instead of the shared default list.🤖 Prompt for AI Agents